Skip to content

Commit

Permalink
Sorting fix for 10+ groups in tidy_ard_column_order() (#353)
Browse files Browse the repository at this point in the history
**What changes are proposed in this pull request?**
* The `tidy_ard_column_order()` now correctly orders grouping columns
when there are 10+ groups. This also corrects an issue in the
hierarchical functions where the ordering of the variables matters.
(#352)

**Reference GitHub issue associated with pull request.** _e.g., 'closes
#<issue number>'_
closes #352


--------------------------------------------------------------------------------

Pre-review Checklist (if item does not apply, mark is as complete)
- [ ] **All** GitHub Action workflows pass with a ✅
- [ ] PR branch has pulled the most recent updates from master branch:
`usethis::pr_merge_main()`
- [ ] If a bug was fixed, a unit test was added.
- [ ] Code coverage is suitable for any new functions/features
(generally, 100% coverage for new code): `devtools::test_coverage()`
- [ ] Request a reviewer

Reviewer Checklist (if item does not apply, mark is as complete)

- [ ] If a bug was fixed, a unit test was added.
- [ ] Run `pkgdown::build_site()`. Check the R console for errors, and
review the rendered website.
- [ ] Code coverage is suitable for any new functions/features:
`devtools::test_coverage()`

When the branch is ready to be merged:
- [ ] Update `NEWS.md` with the changes from this pull request under the
heading "`# cards (development version)`". If there is an issue
associated with the pull request, reference it in parentheses at the end
update (see `NEWS.md` for examples).
- [ ] **All** GitHub Action workflows pass with a ✅
- [ ] Approve Pull Request
- [ ] Merge the PR. Please use "Squash and merge" or "Rebase and merge".
  • Loading branch information
ddsjoberg authored Nov 4, 2024
1 parent cd1e831 commit 2ce4118
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 12 deletions.
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# cards 0.3.0.9002

* The `tidy_ard_column_order()` now correctly orders grouping columns when there are 10+ groups. This also corrects an issue in the hierarchical functions where the ordering of the variables matters. (#352)

* No longer exporting functions `check_pkg_installed()`, `is_pkg_installed()`, `get_min_version_required()`, `get_pkg_dependencies()`. These functions are now internal-only. (#330)

# cards 0.3.0
Expand Down
1 change: 1 addition & 0 deletions R/tidy_ard_order.R
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ tidy_ard_column_order <- function(x) {
group_cols <- dplyr::select(x, all_ard_groups()) |>
names() |>
sort()
group_cols <- group_cols[order(str_extract_all(group_cols, "\\d+") |> unlist() |> as.integer())]

dplyr::select(
x,
Expand Down
12 changes: 0 additions & 12 deletions tests/testthat/_snaps/check_pkg_installed.md

This file was deleted.

32 changes: 32 additions & 0 deletions tests/testthat/test-ard_hierarchical.R
Original file line number Diff line number Diff line change
Expand Up @@ -326,3 +326,35 @@ test_that("ard_hierarchical_count() errors with incomplete factor columns", {
)
)
})

test_that("ard_hierarchical_count() provides correct results with 10+ groups", {
skip_if_not(is_pkg_installed("withr"))
withr::local_seed(1)

expect_silent(
ard <- data.frame(
x1 = sample(LETTERS[1:2], 30, replace = TRUE),
x2 = sample(LETTERS[3:4], 30, replace = TRUE),
x3 = sample(LETTERS[5:6], 30, replace = TRUE),
x4 = sample(LETTERS[7:8], 30, replace = TRUE),
x5 = sample(LETTERS[9:10], 30, replace = TRUE),
x6 = sample(LETTERS[11:12], 30, replace = TRUE),
x7 = sample(LETTERS[13:14], 30, replace = TRUE),
x8 = sample(LETTERS[15:16], 30, replace = TRUE),
x9 = sample(LETTERS[17:18], 30, replace = TRUE),
x10 = sample(LETTERS[19:20], 30, replace = TRUE)
) %>%
ard_hierarchical_count(data = ., variables = names(.))
)

expect_equal(
dplyr::select(ard, all_ard_groups(), all_ard_variables()) |>
names(),
c('group1', 'group1_level', 'group2', 'group2_level', 'group3', 'group3_level',
'group4', 'group4_level', 'group5', 'group5_level', 'group6', 'group6_level',
'group7', 'group7_level', 'group8', 'group8_level', 'group9', 'group9_level',
'variable', 'variable_level')
)

expect_equal(ard[["variable"]][[1]], "x10")
})
32 changes: 32 additions & 0 deletions tests/testthat/test-tidy_ard_column_order.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
test_that("tidy_ard_column_order() works", {
skip_if_not(is_pkg_installed("withr"))
withr::local_seed(1)

# ensure 10+ groups are ordered correctly
expect_equal(
data.frame(
x1 = sample(LETTERS[1:2], 30, replace = TRUE),
x2 = sample(LETTERS[3:4], 30, replace = TRUE),
x3 = sample(LETTERS[5:6], 30, replace = TRUE),
x4 = sample(LETTERS[7:8], 30, replace = TRUE),
x5 = sample(LETTERS[9:10], 30, replace = TRUE),
x6 = sample(LETTERS[11:12], 30, replace = TRUE),
x7 = sample(LETTERS[13:14], 30, replace = TRUE),
x8 = sample(LETTERS[15:16], 30, replace = TRUE),
x9 = sample(LETTERS[17:18], 30, replace = TRUE),
x10 = sample(LETTERS[19:20], 30, replace = TRUE),
dummy = 1L
) |>
ard_categorical(
variables = "dummy",
strata = x1:x10,
statistic = everything() ~ "n"
) |>
dplyr::select(all_ard_groups(), all_ard_variables()) |>
names(),
c('group1', 'group1_level', 'group2', 'group2_level', 'group3', 'group3_level',
'group4', 'group4_level', 'group5', 'group5_level', 'group6', 'group6_level',
'group7', 'group7_level', 'group8', 'group8_level', 'group9', 'group9_level',
'group10', 'group10_level', 'variable', 'variable_level')
)
})

0 comments on commit 2ce4118

Please sign in to comment.