Closes #31 wrapper functions for waist/hip and waist/height ratio

[yurovska]

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 family 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?
• 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<ext> (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)
• Build 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!

[yurovska]

Please don’t judge too harshly, this is my first Pull Request ever.

I would need help with unit tests and other stuff as I have no experience at all.

[yurovska]

@manciniedoardo Need help with {units} package. Checks failed due to its absence within the container.

[manciniedoardo]

@manciniedoardo Need help with {units} package. Checks failed due to its absence within the container.

@AndersAskeland can you look as well pls?

[AndersAskeland]

@yurovska It fails as the units package depends on a system dependency of "udunits2".

This library must be manually installed in Unix (test suite that fails is running this) and MacOS. On Windows it is included in RTools as "bin\udunits2.exe".

As such, we unfortunately cannot use the units package as a dependency. I would actually also argue that even if it did not have a system dependency, we should try refraining from using it, as we should try to limit our dependencies. I have not looked at your code yet, but I am guessing that you use the package to convert between units (m/cm/inches) in cases where waist is collected in a different unit to hip.

I think the concept of allowing different units is important, as one cannot always control what the collected/SDTM units are. I would actually argue that this functionality should be included in admiral base at some point

As for alternative approaches than using the units package, I can personally think of two potential solutions. Either we do manual unit conversion inside the wrapper functions (probably messy to have all potential units conversions inside derive_param_ratio()) or we create some internal data that controls unit conversions and do this in derive_param_ratio(). In either case, I would base the potential unit conversions on the permissible units defined in the CDISC unit codelist.

I think that we might need to discuss this before implementation though, and maybe create a separate issue and pull request.

[yurovska]

This library must be manually installed in Unix (test suite that fails is running this) and MacOS. On Windows it is included in RTools as "bin\udunits2.exe".

@AndersAskeland I'm on MacOS and just did install.packages("units") with no additional steps. All good.

As such, we unfortunately cannot use the units package as a dependency. I would actually also argue that even if it did not have a system dependency, we should try refraining from using it, as we should try to limit our dependencies.

It's a pity. {units} really works like a magic. I simply set the units from AVALU (or admiral::extract_unit(PARAM)) to the context of AVAL in a preliminary step and the conversion performs automatically when AVAL is used in the formula, so that the values of two input parameters are unified prior to Ratio derivation. Then I remove units from AVAL context before returning the result. That easy.

[yurovska]

@AndersAskeland Just tested it also within Roche environment (Ubuntu 22.04 Jammy). No issues.

Do you really think udunits2 might be missing in any environment except GitHub CI/CD?

> renv::install("units", repos = "https://packages.roche.com/Validated-4.3/latest")
# Downloading packages -------------------------------------------------------
- Downloading units from CRAN ...               OK [file is up to date]
Successfully downloaded 1 package in 1.3 seconds.

The following package(s) will be installed:
- units [0.8-5]
These packages will be installed into "~/R/x86_64-pc-linux-gnu-library/4.3".

Do you want to proceed? [Y/n]: Y

# Installing packages --------------------------------------------------------
- Installing units ...                          OK [installed binary and cached in 1.9s]
Successfully installed 1 package in 2 seconds.
> library(units)
udunits database from /usr/share/xml/udunits/udunits2.xml
> val_cm <- set_units(1:10, cm)
> val_cm
Units: [cm]
 [1]  1  2  3  4  5  6  7  8  9 10
> val_mm <- set_units(1:10, mm)
> val_mm
Units: [mm]
 [1]  1  2  3  4  5  6  7  8  9 10
> val_cm - val_mm
Units: [cm]
 [1] 0.9 1.8 2.7 3.6 4.5 5.4 6.3 7.2 8.1 9.0

[AndersAskeland]

@AndersAskeland Just tested it also within Roche environment (Ubuntu 22.04 Jammy). No issues.

Do you really think udunits2 might be missing in any environment except GitHub CI/CD?

I would guess most compute environment have it installed. However, I still feel that we cannot expect all users to have this library or possibility of building it from source.

I noticed that in our compute environment, when first installing units, udunits2 is built from source using the g++/GCC compiler.

So while it might usually work, we would put a requirement on users to have a working C compiler, which is not always the case.

For example for Mac, I think the brew process (or MacOS developer tools) installs the required compilers, however, if you just install R with nothing else, I am unsure if it will work. There might also be a difference on ARM Macs.

If we however take a step back and look at what it is used for, is it only for converting meters/mm/cm/inches? Do you use it for more?

[yurovska]

@AndersAskeland Re-written without use of {units} package. However, only units of length are supported now.

[AndersAskeland]

@yurovska Nice! I will start looking at the pull request. It might take some time, as this is the first time reviewing functions in Admiral and I need to figure out the general layout and stuff :)

[manciniedoardo]

Thanks @yurovska! I left some general comments; @AndersAskeland will do a technical review.

Additionally:

• please take this opportunity to remove hello_admiral() and related tests/documentation as we now have a real function.
• please ensure the functions appear on the reference page and take the opportunity to tidy up the reference page if you can - happy to help on this front if needed.
• add tests for the ratio function too, otherwise the test coverage for the package will be low.

[yurovska]

• please take this opportunity to remove hello_admiral() and related tests/documentation as we now have a real function.
• please ensure the functions appear on the reference page and take the opportunity to tidy up the reference page if you can - happy to help on this front if needed.
• add tests for the ratio function too, otherwise the test coverage for the package will be low.

@manciniedoardo Thanks for your review. I already started addressing commit suggestions. Will be ready soon. Regarding the bullet points above:

• Sure, will remove the dummy function.
• The functions do appear on the reference page. Checked via devtools::build_site(). As for tidying up, I only updated the section related to the scope of this PR. If it requires changes in general, I think it should be a separate Issue.
• I actually tried to add unit tests for derive_param_ratio but R CMD check failed because testthat was not able to find a function, which is not exported. Perhaps, internal functions do not require testing since it's covered by unit tests of the exported functions that use the internal ones, so it won't affect the overall test coverage rate. Please let me know if you think otherwise.

[AndersAskeland]

• I actually tried to add unit tests for derive_param_ratio but R CMD check failed because testthat was not able to find a function, which is not exported. Perhaps, internal functions do not require testing since it's covered by unit tests of the exported functions that use the internal ones, so it won't affect the overall test coverage rate. Please let me know if you think otherwise.

I would still think we need to do something for non-exported functions, especially on derive_var_ratio, as it is somewhat advanced. I also believe that covr by default include non-exported functions. How are you running the R CMD check? Is it working when running devtools::check()?

[yurovska]

@AndersAskeland @manciniedoardo Thanks again for your valuable review. I think I'm done addressing all your comments and the PR is ready for the next (or maybe final) round. I haven't resolved some conversations, please do so if you agree with the update made.

[manciniedoardo]

LGTM - a couple of minor comments to implement but then I am happy from a documentation and user side. @AndersAskeland and @pharmaverse/admiralmetabolic pls add your final feedback and any technical by end of week so we can then merge.

[AndersAskeland]

Sorry for my late review, I was suddenly required to travel out of office for most of the week. I will quickly go through it tomorrow :) I have been thinking about one thing though, which I think we shouldn’t necessarily change now, but could alter in the future depending on your thoughts on it.

My thoughts have been about the design/implementation of the actual conversion, and weather that should be something associated to the actual weight height / waist hip ratio, or should be more general and associated with the ratio function (which it is now). We had a short discussion about me wanting the conversion function to fail if conversion was not supported, but you (rightly) pointed out that there are ratios that we should not convert (for instance kg/m).

I still agree that this is something we should cater to . However, I am thinking about cases where you might be interested in a ratio with convertible units. For instance a meter / centimetre ratio. I realise that this is very uncommon and strange, but my thoughts are then, who are we to decide which ratios to allow?

Hence, I think we should at some point discuss if we should keep the ratio function extremely generic and move everthing associated to conversion to the wrapper function. In some sense I think this makes more sense, because, we would not un necessarily call a convert function when not needed, and we could be more consistent with the return from the unit conversion function. I am not a big fan of a function being able to return NAs.

But… For time sakes, and to allow us to implement this before next week, I think we can discuss this later as it would not actually impede the functions to change the implementation.

[yurovska]

Sorry for my late review, I was suddenly required to travel out of office for most of the week. I will quickly go through it tomorrow :) I have been thinking about one thing though, which I think we shouldn’t necessarily change now, but could alter in the future depending on your thoughts on it.

My thoughts have been about the design/implementation of the actual conversion, and weather that should be something associated to the actual weight height / waist hip ratio, or should be more general and associated with the ratio function (which it is now). We had a short discussion about me wanting the conversion function to fail if conversion was not supported, but you (rightly) pointed out that there are ratios that we should not convert (for instance kg/m).

I still agree that this is something we should cater to . However, I am thinking about cases where you might be interested in a ratio with convertible units. For instance a meter / centimetre ratio. I realise that this is very uncommon and strange, but my thoughts are then, who are we to decide which ratios to allow?

Hence, I think we should at some point discuss if we should keep the ratio function extremely generic and move everthing associated to conversion to the wrapper function. In some sense I think this makes more sense, because, we would not un necessarily call a convert function when not needed, and we could be more consistent with the return from the unit conversion function. I am not a big fan of a function being able to return NAs.

But… For time sakes, and to allow us to implement this before next week, I think we can discuss this later as it would not actually impede the functions to change the implementation.

Thanks in advance for finding time to finalize the review.

I got your point and I think I have an idea. What if we add a sort of unit_conversion argument (TRUE/FALSE) in derive_param_ratio that would be disabled by default, so that a developer of a wrapper function decides whether to call it with unit_conversion = TRUE if applicable for a particular ratio parameter? With this approach I'd agree to throw an error if conversion factor is not found instead of returning NA.

[AndersAskeland]

Amazing work @yurovska !

I think it is ready to merge now!

I have a couple of questions/comments, but they are more some general considerations that I don't think we should do anything about from our side :)

[AndersAskeland]

Sorry for my late review, I was suddenly required to travel out of office for most of the week. I will quickly go through it tomorrow :) I have been thinking about one thing though, which I think we shouldn’t necessarily change now, but could alter in the future depending on your thoughts on it.
My thoughts have been about the design/implementation of the actual conversion, and weather that should be something associated to the actual weight height / waist hip ratio, or should be more general and associated with the ratio function (which it is now). We had a short discussion about me wanting the conversion function to fail if conversion was not supported, but you (rightly) pointed out that there are ratios that we should not convert (for instance kg/m).
I still agree that this is something we should cater to . However, I am thinking about cases where you might be interested in a ratio with convertible units. For instance a meter / centimetre ratio. I realise that this is very uncommon and strange, but my thoughts are then, who are we to decide which ratios to allow?
Hence, I think we should at some point discuss if we should keep the ratio function extremely generic and move everthing associated to conversion to the wrapper function. In some sense I think this makes more sense, because, we would not un necessarily call a convert function when not needed, and we could be more consistent with the return from the unit conversion function. I am not a big fan of a function being able to return NAs.
But… For time sakes, and to allow us to implement this before next week, I think we can discuss this later as it would not actually impede the functions to change the implementation.

Thanks in advance for finding time to finalize the review.

I got your point and I think I have an idea. What if we add a sort of unit_conversion argument (TRUE/FALSE) in derive_param_ratio that would be disabled by default, so that a developer of a wrapper function decides whether to call it with unit_conversion = TRUE if applicable for a particular ratio parameter? With this approach I'd agree to throw an error if conversion factor is not found instead of returning NA.

That is a good idea! That solution also has the benefit of us not needing to repeat ourselves in the "specific" functions (by needing to have code for the conversion function in both function).

[yurovska]

That is a good idea! That solution also has the benefit of us not needing to repeat ourselves in the "specific" functions (by needing to have code for the conversion function in both function).

I'll keep that in mind and implement as part of moving from the internal assert_unit() to the admiraldev's one.

[yurovska]

Amazing work @yurovska !

I think it is ready to merge now!

I have a couple of questions/comments, but they are more some general considerations that I don't think we should do anything about from our side :)

Thanks again for your review. Questions answered. Conversations resolved. We're good to go merging.