Closes #2492 use ANRIND for abnormal baseline

[millerg23]

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 main 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?
• Review the Cheat Sheet. Make any required updates to it by editing the file inst/cheatsheet/admiral_cheatsheet.pptx and re-upload a PDF and a PNG version of it to the same folder. (The PNG version can be created by taking a screenshot of the PDF version.)
• 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 under the header # admiral (development version) if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers). A Developer Notes section is available in NEWS.md for tracking developer-facing issues.
• 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	97%	✔
Summary	97% (5064 / 5226)	✔

[bundfussr]

I'm not sure if it is a good idea to use explicit conditions based on BNRIND because it's values are not standardized. In the definitions it is assumed that "HIGH" indicates an abnormal high value. If this is not the case, the grading variables are created without an error or warning but the results are wrong. But they are not obviously wrong, i.e., without double-programming the grading variables it will most likely be missed.

Maybe the condition for baseline abnormal should be specified by the user.

What do you think?

[millerg23]

I'm not sure if it is a good idea to use explicit conditions based on BNRIND because it's values are not standardized. In the definitions it is assumed that "HIGH" indicates an abnormal high value. If this is not the case, the grading variables are created without an error or warning but the results are wrong. But they are not obviously wrong, i.e., without double-programming the grading variables it will most likely be missed.

Maybe the condition for baseline abnormal should be specified by the user.

What do you think?

Would we have a default value, in derive_var_atoxgr_dir() function? Or always force user to define whats abnormal? Also, we might need 2 inputs one for HYPER (ie looking at high values, so BNRIND == "HIGH" is abnormal), and one for HYPO (looking at low values, so BNRIND == "LOW" is abnormal). This only effects 5 lab tests that are all looking at HIGH in NCICTCAEv5, but this could change in future if we added more grading criteria.

[bundfussr]

I'm not sure if it is a good idea to use explicit conditions based on BNRIND because it's values are not standardized. In the definitions it is assumed that "HIGH" indicates an abnormal high value. If this is not the case, the grading variables are created without an error or warning but the results are wrong. But they are not obviously wrong, i.e., without double-programming the grading variables it will most likely be missed.
Maybe the condition for baseline abnormal should be specified by the user.
What do you think?

Would we have a default value, in derive_var_atoxgr_dir() function? Or always force user to define whats abnormal? Also, we might need 2 inputs one for HYPER (ie looking at high values, so BNRIND == "HIGH" is abnormal), and one for HYPO (looking at low values, so BNRIND == "LOW" is abnormal). This only effects 5 lab tests that are all looking at HIGH in NCICTCAEv5, but this could change in future if we added more grading criteria.

I would not set a default value because then we get the same problem as before. I.e., users just use the default and don't notice that the results are wrong.

[millerg23]

I'm not sure if it is a good idea to use explicit conditions based on BNRIND because it's values are not standardized. In the definitions it is assumed that "HIGH" indicates an abnormal high value. If this is not the case, the grading variables are created without an error or warning but the results are wrong. But they are not obviously wrong, i.e., without double-programming the grading variables it will most likely be missed.
Maybe the condition for baseline abnormal should be specified by the user.
What do you think?

Would we have a default value, in derive_var_atoxgr_dir() function? Or always force user to define whats abnormal? Also, we might need 2 inputs one for HYPER (ie looking at high values, so BNRIND == "HIGH" is abnormal), and one for HYPO (looking at low values, so BNRIND == "LOW" is abnormal). This only effects 5 lab tests that are all looking at HIGH in NCICTCAEv5, but this could change in future if we added more grading criteria.

I would not set a default value because then we get the same problem as before. I.e., users just use the default and don't notice that the results are wrong.

Yes, agreed. So do you think allowing users to pass in values of BNRIND that are deemed abnormal for HYPER (HIGH values) lab tests would be sufficient.
An argument into derive_var_atoxgr_dir() called something like, bnrind_abnormal_values user could pass in "HIGH" or "H" or c("HIGH", "HIGH HIGH")

[bundfussr]

I'm not sure if it is a good idea to use explicit conditions based on BNRIND because it's values are not standardized. In the definitions it is assumed that "HIGH" indicates an abnormal high value. If this is not the case, the grading variables are created without an error or warning but the results are wrong. But they are not obviously wrong, i.e., without double-programming the grading variables it will most likely be missed.
Maybe the condition for baseline abnormal should be specified by the user.
What do you think?

Would we have a default value, in derive_var_atoxgr_dir() function? Or always force user to define whats abnormal? Also, we might need 2 inputs one for HYPER (ie looking at high values, so BNRIND == "HIGH" is abnormal), and one for HYPO (looking at low values, so BNRIND == "LOW" is abnormal). This only effects 5 lab tests that are all looking at HIGH in NCICTCAEv5, but this could change in future if we added more grading criteria.

I would not set a default value because then we get the same problem as before. I.e., users just use the default and don't notice that the results are wrong.

Yes, agreed. So do you think allowing users to pass in values of BNRIND that are deemed abnormal for HYPER (HIGH values) lab tests would be sufficient. An argument into derive_var_atoxgr_dir() called something like, bnrind_abnormal_values user could pass in "HIGH" or "H" or c("HIGH", "HIGH HIGH")

Most likely this would be sufficient. However, I would add "high" to the argument name. Then it is clearer and we don't need to rename it if in future we need an argument for low abnormalities. Maybe we could also remove the "b". Then we could use it also if we need to use ANRIND in the future. So for example we could rename it to nrind_abnormal_high_values.

[millerg23]

@bundfussr @bms63 can you look at recent updates. We only call the function for HIGH or LOW, so will only need one variable to capture LOW and HIGH. I updated function to check if BNRIND is a required variable for a lab test, then abnormal_indicator is required. You can see this in the unit tests, I only populate abnormal_indicator for the 5 lab tests that are affected, and the others run as normal with it set to NULL.
If you both are happy with changes I probably need to add something to the Vignette to mention the update.

[bundfussr]

@bundfussr @bms63 can you look at recent updates. We only call the function for HIGH or LOW, so will only need one variable to capture LOW and HIGH. I updated function to check if BNRIND is a required variable for a lab test, then abnormal_indicator is required. You can see this in the unit tests, I only populate abnormal_indicator for the 5 lab tests that are affected, and the others run as normal with it set to NULL. If you both are happy with changes I probably need to add something to the Vignette to mention the update.

@millerg23 , looks good to me. I assume a scalar is expected for abnormal_indicator because we can't have "HIGH HIGH" for BNRIND.

[bms63]

if the list of variables is fixed (Alanine aminotransferase increased, Alkaline phosphatase increased, Aspartate aminotransferase increased, Blood bilirubin increased and GGT Increased) should we do anything to check their presence, alert user to missing or some other skullduggery?

[millerg23]

If BNRIND is a required variable, as it is for those 5 lab tests, then as well as checking for existence on line 222, I then check that abnormal_indicator is not NULL.

[bms63]

Are all 5 labs covered for testing?

[millerg23]

Yes, see diffs in unit tests where I have added abnormal_indicator = "HIGH", to the function call, should be in 5 places. ALT, AST, ALKPH, BILI, GGT.

[millerg23]

@bundfussr @bms63 can you look at recent updates. We only call the function for HIGH or LOW, so will only need one variable to capture LOW and HIGH. I updated function to check if BNRIND is a required variable for a lab test, then abnormal_indicator is required. You can see this in the unit tests, I only populate abnormal_indicator for the 5 lab tests that are affected, and the others run as normal with it set to NULL. If you both are happy with changes I probably need to add something to the Vignette to mention the update.

@millerg23 , looks good to me. I assume a scalar is expected for abnormal_indicator because we can't have "HIGH HIGH" for BNRIND.

Ah!! It needs to be a character vector, right? As c("HIGH", "HIGH HIGH") could be a valid entry, or in odd cases c("H", "HIGH"). I will update metadata to use BNRIND %in% abnormal_indicator (although will that work with single values??, need to test).

[bundfussr]

Ah!! It needs to be a character vector, right? As c("HIGH", "HIGH HIGH") could be a valid entry, or in odd cases c("H", "HIGH"). I will update metadata to use BNRIND %in% abnormal_indicator (although will that work with single values??, need to test).

R doesn't distinguish vectors and scalars, i.e., BNRIND %in% abnormal_indicator works with single values because R considers them as a vector of length 1.

I think it makes sense to accept character vectors. Then we don't need to update if users need something like c("HIGH", "HIGH HIGH").

[millerg23]

thanks for initial QC, think I have made all updates now.
I have used abnormal_indicator in the metadata for all 5 tests now, and used %in% so can read a character vector.
I updated unit tests for the 5 lab tests, and used different combinations, I used combination of "HIGH", "HIGH HIGH" or c("H", "HIGH").
I also updated header for derive_var_atoxgr_dir() a bit more for the new argument, and updated the Vignette. @bms63 can you do a sanity check on these, please.

[bms63]

thanks for initial QC, think I have made all updates now. I have used abnormal_indicator in the metadata for all 5 tests now, and used %in% so can read a character vector. I updated unit tests for the 5 lab tests, and used different combinations, I used combination of "HIGH", "HIGH HIGH" or c("H", "HIGH"). I also updated header for derive_var_atoxgr_dir() a bit more for the new argument, and updated the Vignette. @bms63 can you do a sanity check on these, please.

I don't see any updates for the Vignette? did you mean documenation?

[millerg23]

thanks for initial QC, think I have made all updates now. I have used abnormal_indicator in the metadata for all 5 tests now, and used %in% so can read a character vector. I updated unit tests for the 5 lab tests, and used different combinations, I used combination of "HIGH", "HIGH HIGH" or c("H", "HIGH"). I also updated header for derive_var_atoxgr_dir() a bit more for the new argument, and updated the Vignette. @bms63 can you do a sanity check on these, please.

I don't see any updates for the Vignette? did you mean documenation?

Apologies, my push failed!! Its there now...

[bms63]

@zdz2101 can we get your eyes on these lab updates as well?

[bms63]

Suggested change

	- fixed for `TERM = "INR increased"`, criteria was wrongly using `x ULN`, for first part of criteria for grades 1 to 3. For example, `">2.5 x ULN"` changed to `">2.5"` for grade 3. (#2534).
	- fix for `TERM = "INR increased"`, criteria was wrongly using `x ULN`, for first part of criteria for grades 1 to 3. For example, `">2.5 x ULN"` changed to `">2.5"` for grade 3. (#2534).