Closes #1966 address derive_vars_joined bugs

[zdz2101]

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the devel branch until you have checked off each task.

• Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
• Updated relevant unit tests or have written new unit tests, which should consider realistic data scenarios and edge cases, e.g. empty datasets, errors, boundary cases etc. - See Unit Test Guide
• If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
• Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
• Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• Address any updates needed for vignettes and/or templates
• Update NEWS.md if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers)
• Build admiral site pkgdown::build_site() and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.
• Address or fix all lintr warnings and errors - lintr::lint_package()
• Run R CMD check locally and address all errors and warnings - devtools::check()
• Link the issue in the Development Section on the right hand side.
• Address all merge conflicts and resolve appropriately
• Pat yourself on the back for a job well done! Much love to your accomplishment!

[github-actions]

Package	Line Rate	Health
admiral	99%	✔
Summary	99% (4213 / 4271)	✔

[zdz2101]

@bundfussr This modification addresses the get_duplicates reprex you cited in the issue, though I'm not sure this is the "perfect" solution to it. There are 3 places where check_type for the duplicates are being checked, up at line 364 (which is set to "none"), here in line 402 (which is passed through the user in the function call), and passively through derive_vars_merged() in line 413 (which is defaulted to "warning"). I think(?) the intention was to allow duplicates (& as a result suppress messaging) up until the derive_vars_merged() call in the function body where there is a specific duplciate_msg to highlight the "final" duplicates, which is why I added the new line.

May be important to note, that in our vignette generic.Rmd, we have check_type = "none" in one of the examples to suppress what would've been a similar warning you have in your reprex, because the default for check_type right now in derive_vars_joined() is "warning". But with the proposed fix, you can actually remove this line and let it be the default.

[bundfussr]

This line should not be changed. Otherwise, no message is issued if the selected observation is not unique. Consider for example

derive_vars_joined(
  adlb_ast,
  dataset_add = adlb_tbili_pbl,
  by_vars = exprs(STUDYID, USUBJID),
  order = exprs(ADT),
  new_vars = exprs(TBILI_ADT = ADT, TBILI_SEQ = ASEQ),
  filter_join = ADT <= ADT.join,
  mode = "first"
)

with the datasets from the second example in the issue. Here a warning should be issued because the first TBILI assessment at or after the AST assessment is not unique for ASEQ %in% c(2, 3).

[zdz2101]

If thats the case, is the example in the generic.Rmd a little misleading then? If it weren't toggled to check_type = "none" there would be a warning.

I also think I'm a little confused how order is handled in general:

In general, !!!chr2vars(names(order)) will not select columns unless user provides a named value so the common_vars statement a little further down can be inconsistent. Should it be swapped out with (?):

• replace_values_by_names
• extract_vars()

[bundfussr]

Specifying check_type = "none" in the example makes sense because only the maximum severity is added as new variable. If there are multiple observations with the maximum severity, it doesn't matter which one is used.

Regarding order, we need to more comments to the code. The purpose of the mutate() call is to create variables for named expressions in order. Then these can be used in join_vars or the filter condition.
You are right that !!!chr2vars(names(order)) is not correct. We may lose the variables used in the expressions of order. I think it should be replaced by !!!replace_values_by_names(extract_vars(order)).

[zdz2101]

Looks like that did the trick! Now retaining the original check_type = check_type code and using the !!!replace_values_by_names(extract_vars(order)) fixes the reprex you provided in the issue and still provides the appropriate warning message you indicated 3 replies above

[zdz2101]

@bundfussr this one addresses the NULL new_vars issue, don't think it's the most "elegant" but should work...?

[bundfussr]

I think it is not in line with the documentation and intended behavior. If new_vars = NULL is specified, all variables from dataset_add should be added. However, in the example from the issue no variable is added. Of course, we can not add all variables because they are already in the input dataset. Thus an error should be issued.

[zdz2101]

So in the case of all variables already in the input dataset, the desired result would be an error and no-return object at all?

[bundfussr]

Yes, or more precise, if any of the variables specified for new_vars is already in the input dataset, the function should stop with an error.

[zdz2101]

Please see updated if (is.null(new_vars)) at line 353 that implements suggestion and updated test with the corresponding messaging

[bms63]

The examples that @bundfussr provided in the issue - is it wise to move them into the tests?

• Slight misspelling in the news file.

[bms63]

Thanks for adding the tests! @bundfussr anymore concerns?

[bundfussr]

I think it is not in line with the documentation and intended behavior. If new_vars = NULL is specified, all variables from dataset_add should be added. However, in the example from the issue no variable is added. Of course, we can not add all variables because they are already in the input dataset. Thus an error should be issued.

[bundfussr]

This line should not be changed. Otherwise, no message is issued if the selected observation is not unique. Consider for example

derive_vars_joined(
  adlb_ast,
  dataset_add = adlb_tbili_pbl,
  by_vars = exprs(STUDYID, USUBJID),
  order = exprs(ADT),
  new_vars = exprs(TBILI_ADT = ADT, TBILI_SEQ = ASEQ),
  filter_join = ADT <= ADT.join,
  mode = "first"
)

with the datasets from the second example in the issue. Here a warning should be issued because the first TBILI assessment at or after the AST assessment is not unique for ASEQ %in% c(2, 3).