Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General Issue: derive_vars_transposed() shouldn't produce list columns #2563

Closed
bundfussr opened this issue Nov 14, 2024 · 2 comments · Fixed by #2592, #2611 or #2625
Closed

General Issue: derive_vars_transposed() shouldn't produce list columns #2563

bundfussr opened this issue Nov 14, 2024 · 2 comments · Fixed by #2592, #2611 or #2625

Comments

@bundfussr
Copy link
Collaborator

bundfussr commented Nov 14, 2024

Background Information

The functions derive_vars_transposed() and derive_vars_atc() may produce list columns.

Consider for example the ATC variables derivation in the OCCDS vignette:

derive_vars_atc(cm, facm)
#> Warning: Values from `FASTRESC` are not uniquely identified; output will contain
#> list-cols.
#> • Use `values_fn = list` to suppress this warning.
#> • Use `values_fn = {summary_fun}` to summarise duplicates.
#> • Use the following dplyr code to identify duplicates.
#>   {data} |>
#>   dplyr::summarise(n = dplyr::n(), .by = c(USUBJID, FAREFID, FATESTCD)) |>
#>   dplyr::filter(n > 1L)
#> # A tibble: 3 × 8
#>   USUBJID      CMGRPID CMREFID CMDECOD        ATC1CD    ATC2CD    ATC3CD ATC4CD
#>   <chr>        <chr>   <chr>   <chr>          <list>    <list>    <list> <list>
#> 1 BP40257-1001 14      1192056 PARACETAMOL    <chr [1]> <chr [1]> <chr>  <chr> 
#> 2 BP40257-1001 18      2007001 SOLUMEDROL     <chr [3]> <chr [3]> <chr>  <chr> 
#> 3 BP40257-1002 19      2791596 SPIRONOLACTONE <chr [1]> <chr [1]> <chr>  <chr>

The issue is that id_vars = exprs(FAGRPID) is missing in the call.

However, this is not clear from the warning. Furthermore, admiral functions shouldn't produce list columns.

Thus, the uniqueness should be checked in derive_vars_transposed() such that an error is issued if the input results in list columns.

Definition of Done

An error is issued if the input of derive_vars_transposed() results in list columns.

@bundfussr bundfussr self-assigned this Nov 21, 2024
@bundfussr bundfussr moved this from Backlog to In Progress in admiral (sdtm/adam, dev, ci, template, core) Dec 2, 2024
@bundfussr bundfussr linked a pull request Dec 2, 2024 that will close this issue
15 tasks
@bundfussr bundfussr moved this from In Progress to In Review in admiral (sdtm/adam, dev, ci, template, core) Dec 2, 2024
bundfussr added a commit that referenced this issue Dec 13, 2024
@bms63 bms63 closed this as completed in e2415a0 Dec 13, 2024
@bundfussr
Copy link
Collaborator Author

The check on duplicates should be performed after applying the filter.

@bundfussr bundfussr reopened this Dec 16, 2024
@bundfussr bundfussr moved this from Priority to In Progress in admiral (sdtm/adam, dev, ci, template, core) Dec 16, 2024
@bundfussr bundfussr linked a pull request Dec 16, 2024 that will close this issue
15 tasks
@bundfussr bundfussr moved this from In Progress to In Review in admiral (sdtm/adam, dev, ci, template, core) Dec 16, 2024
bundfussr added a commit that referenced this issue Dec 17, 2024
* #2563 no_list_columns: add check to avoid list columns

* #2563 no_list_columns: update documentation

* #2563 no_list_columns: fix example and add see also

* #2563 no_list_columns: avoid package name prefix

* #2563 no_list_columns: update required admiraldev version

* #2563 no_list_columns: split files (and clean up tests)

* #2563 no_list_columns: update man

* #2563 no_list_columns: filter before check

---------

Co-authored-by: Daniel Sjoberg <danield.sjoberg@gmail.com>
@bundfussr
Copy link
Collaborator Author

I missed to add a snapshot.

@bundfussr bundfussr reopened this Jan 7, 2025
bundfussr added a commit that referenced this issue Jan 7, 2025
@bundfussr bundfussr linked a pull request Jan 7, 2025 that will close this issue
15 tasks
bms63 pushed a commit that referenced this issue Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment