Add metrics parameter to data_tabulate()#689
Conversation
There was a problem hiding this comment.
On top of the other comments to address above, please add a bullet point in NEWS.md and add an example for the new argument in the docs of data_tabulate().
(@strengejacke I hid all the copilot comments because they were just duplicates of your comments and adding noise to this PR)
|
I was wondering if it should be documented that |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #689 +/- ##
==========================================
+ Coverage 92.09% 92.11% +0.02%
==========================================
Files 76 76
Lines 7727 7766 +39
==========================================
+ Hits 7116 7154 +38
- Misses 611 612 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
etiennebacher
left a comment
There was a problem hiding this comment.
I was wondering if it should be documented that measures = c() will return the frequencies only.
Yes I think so.
Also, that made me wonder if "N" should also be optional.
I don't know about that but if it's not included then I think this measures parameter should be renamed shares. @strengejacke WDYT?
The workflow for code styling fails. We use Air to automatically format code so you can install it and run it on the project to fix this failure.
| #' `NULL`. Can be `"raw"` (includes `NA` values), `"valid"` (excludes `NA` values) | ||
| #' or `"cumulative"` (excludes `NA` values). |
There was a problem hiding this comment.
| #' `NULL`. Can be `"raw"` (includes `NA` values), `"valid"` (excludes `NA` values) | |
| #' or `"cumulative"` (excludes `NA` values). | |
| #' `NULL`. Can be several choices among `"raw"` (includes `NA` values), | |
| #' `"valid"` (excludes `NA` values) and `"cumulative"` (excludes `NA` values). |
There was a problem hiding this comment.
I changed the wording but not exactly to yours. See what you think.
| * `data_tabulate()` gain a `measures` argument to allow selection of columns to | ||
| display ("raw", "valid", and "cumulative") (#689). |
There was a problem hiding this comment.
You can add your github username if you want: (#689, <username>)
measures parameter to data_tabulate()
"shares" is not meaningful to me. Maybe "percentages" would be more descriptive? |
|
If |
This sounds good. Can you let me know if |
I added the "N" (in uppercase) option. I did have to change one additional function that assumed that the N column would be present. All of the tests still pass. On the tests, the linter does not like testing for |
measures parameter to data_tabulate()metrics parameter to data_tabulate()
|
I think everything is resolved unless you have new requests. |
| #' @param metrics Optional character vector, indicating the types of | ||
| #' percents to be included. Only applies to frequencies, i.e. when `by` is | ||
| #' `NULL`. Can include any combination of `N` (frequencies including NA), `"raw"` (includes `NA` values), | ||
| #' `"valid"` (excludes `NA` values) or `"cumulative"` (excludes `NA` values). | ||
| #' Using `c()` will return a table with only the value labels. Invalid | ||
| #' values (`metrics = "foo"`) are silently ignored. |
There was a problem hiding this comment.
| #' @param metrics Optional character vector, indicating the types of | |
| #' percents to be included. Only applies to frequencies, i.e. when `by` is | |
| #' `NULL`. Can include any combination of `N` (frequencies including NA), `"raw"` (includes `NA` values), | |
| #' `"valid"` (excludes `NA` values) or `"cumulative"` (excludes `NA` values). | |
| #' Using `c()` will return a table with only the value labels. Invalid | |
| #' values (`metrics = "foo"`) are silently ignored. | |
| #' @param metrics Optional character vector, indicating the types of | |
| #' metrics to be included. Only applies to frequencies, i.e. when `by` is | |
| #' `NULL`. Can include any combination of `N` (frequencies including `NA`), | |
| #' `"raw"` (percentage including `NA` values), `"valid"` (percentage excluding | |
| #' `NA` values) or `"cumulative"` (percentage excluding `NA` values). |
The part about silently ignoring unknown values seemed wrong (it errors) so I removed it.
| } | ||
| out <- cbind(var_info, out) | ||
| } | ||
| total_n <- sum(out$N, na.rm = TRUE) |
There was a problem hiding this comment.
Why has this line moved? I don't think it causes an issue in itself but I'd rather minimize the git diff.
|
Can you also resolve the conflicts? |
|
I tried running Air on the file but the only issue it identified was duplicate blank lines. Not sure why this is failing. |
|
The problem was that a new version of Air was released a couple of days ago and replaces There are just the few comments above to resolve now (don't worry about linting failures). |
How about having a separate branch for those, since the comments are in unrelated files? |
|
Yes I have actually merged a PR that fixes those Air failures so that these changes don't appear here. Your latest commit message says you left questions on some comments but I don't see anything, am I missing something? I just want to avoid a situation where we're both waiting for something from the other. |
I think, in the end, the only question I had was about metrics = "foo" and that doesn't matter. |
|
There are still unresolved comments above about docs/NEWS/line changed. I have made "suggestions" using Github interface but didn't actually apply them. |
|
I'm not sure why two builds are failing, but they don't seem related. R crashed during the test of Update: ── Warning (test-standardize-data.R:23:1): standardize.numeric ─────────────────
update 2 https://discourse.mc-stan.org/t/global-state-has-changed-warning/36591 Was this resolved for update 3 Apparently the resolved this by deleting |
There were a few ways to approach this. The cumulative values depends on having the valid values, and it could make sense to just set any not included measures to NULL at the end, the way I did for valid.
I added a test for
measures = "raw"... I could add for other variations also.Fixes #685