Close (#22) Created Obesity ADVS Vignette

[Siddhesh2097]

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!

[github-actions]

Package	Line Rate	Health
admiralmetabolic	100%	✔
Summary	100% (3 / 3)	✔

[AndersAskeland]

Amazing job @Siddhesh2097 ! I noticed that the pull-request is still in draft mode though. Are you still working on it, or do you want a review?

[Siddhesh2097]

@AndersAskeland Not able to resolve the pending two checks . Can you help ?

[AndersAskeland]

@Siddhesh2097 I just did a quick check, and it seems to be triggered by using the function derive_vars_crit_flag(). This function is currently only part of the development version of Admiral (i.e. it is not yet released). This is also the reason for the check passing on the development branch but failing on the release and old release branch.

I will check if I can find any good workarounds later today or tomorrow.

[AndersAskeland]

@Siddhesh2097 I just did a quick check, and it seems to be triggered by using the function derive_vars_crit_flag(). This function is currently only part of the development version of Admiral (i.e. it is not yet released). This is also the reason for the check passing on the development branch but failing on the release and old release branch.

I will check if I can find any good workarounds later today or tomorrow.

I am unsure how we proceed for these failed check. If we skip the CI checks for this pull request, we will encounter the same error in all new branches / pull requests from now on (until the function is released in admiral base). We don't want that.

The best alternative I can see would be to temporarily copy / paste derive_vars_crit_flag() from admiral into admiralmetabolic (perhaps in a file like R/temp_ derive_vars_crit_flag.R). Then, for us to remember to remove this function (and change the dependency back to admiral), we could slightly modify the function to check the admiral namespace for the derive_vars_crit_flag(). If it is found it could fail loudly - giving us an heads up to remove the function.

There might still be a potential problem with the checks for the old release of admiral though. However, I am unsure which version the CI check use, and how long until this function propagate down to an old release.

Do you have any other suggestions @manciniedoardo , and do you know how one usually handle the check for an old release in these extension packages?

[AndersAskeland]

While we are waiting to find out how to handle the failing tests, here are some other general comments :)

• You can clean up the _pkgdown.yml file and add this new ADVS vingette to the articles. For some context, the _pkgdown.yml contains the actual structure of how the website will be built. See

  admiralmetabolic/_pkgdown.yml

  Lines 68 to 72 in a3f6666

  	articles:
  	text: User Guides
  	menu:
  	- text: Creating ADXX
  	href: articles/adxx.html

• There are various references to an adxx vingette. I assume that this is an example, and I would remove it now, when we have our first actual vingette.
• The general workflow for vingettes are that they are automatically built on package release. Hence, we would not want people to be able to commit these vingettes (or other documentation) built locally. A way to avoid this is to add the doc and Meta folder to .gitignore. This can be automatically done by running devtools::build_vingettes(). You will also notice that this command updates the .Rbuildignore file with the doc folder. This is needed as the devtools::build_vingettes() function stores the built vingettes in this folder for you to view, and we don't want to include this in the source package.

[manciniedoardo]

@Siddhesh2097 @AndersAskeland I've now fixed the failing checks (thanks @bundfussr for the support) - it was sort of my fault but I can try explain below.

Our CICD actions look for the staged_dependencies.yaml file and use that to install the development version of some packages ({admiral} included) rather than the version off of CRAN, precisely to allow us to use features from those packages while they are still in development (e.g. the criterion flag function).

staged_dependencies.yaml used to be part of {admiraltemplate} (the base repo we use to create our extension packages) before we erroneously removed it prior to creating {admiralmetabolic}. So, I manually re-added it to {admiralmetabolic} but in doing so named it staged_dependencies.yml rather than staged_dependencies.yaml. Because of this, the workflow wasn't finding the file and wasn't installing the right version of {admiral}, hence why it couldn't find the new function.

I've updated the file within this PR so that once merged we don't have this problem anymore.

NB: @AndersAskeland I believe these checks refer to R releases rather than {admiral} releases.

[manciniedoardo]

Thanks @Siddhesh2097 - this is a super first draft!

Along with my review comments, as @AndersAskeland mentioned please could you update _pkgdown.yml to remove references to adxx and sub them with references to advs so that it is possible to navigate to the vignette from the website navbar.

Thanks!

[manciniedoardo]

@kathrinflunkert can you also please review?

[manciniedoardo]

Thanks @Siddhesh2097 - changes look good and i've given it a second pass.

[AndersAskeland]

Sorry for this review being a bit late. I forgot to actually submit it :P

[manciniedoardo]

Some formatting/typo corrections

[kathrinflunkert]

Looks great in general! I was able to follow all steps in the vignette. I only added one comment on the PARAMN.

[manciniedoardo]

HI @Siddhesh2097 - one final review from me. We discussed in the {admiral} core team and with @AndersAskeland and have concluded that in order not to duplicate content, it's ok that the vignette is not completely executable, so if you could please hide the relevant sections that would be super.

Once this is done I think we are ready to merge and I will approve and merge in.

[manciniedoardo]

@pharmaverse/admiralmetabolic - see my comment above regarding the executability of the vignette. Once @Siddhesh2097 addresses the final comments we should be ready to merge, so unless you have any final thoughts, please approve this PR 😄

[AndersAskeland]

Amazing work @Siddhesh2097 ! It is almost there. I have some small suggestions in relation to text/phrasing. Its up to you if you want to implement them.

[kathrinflunkert]

Amazing job Siddhesh!!

[AndersAskeland]

Woho! Great work!

[manciniedoardo]

Super work @Siddhesh2097 😄

[starosto]

I have done review already after the merge and I only have two suggestions:

• in the lookup table (code line 113-115) for the parameters BMI, HIPCIR, WSTCIR, the variable PARCAT1 has the value “Subject Characteristic”, and since weight and circumferences are also measured during the study, I would assign “Vital Sign”
• in the future it might be worth adding parameters derived by the derive_param_computed() function to derive, for example, WSTHIPR (Waist to Hip Ratio [%]) or DTG (Distance to Goal (BMI) [%])

[manciniedoardo]

I have done review already after the merge and I only have two suggestions:

• in the lookup table (code line 113-115) for the parameters BMI, HIPCIR, WSTCIR, the variable PARCAT1 has the value “Subject Characteristic”, and since weight and circumferences are also measured during the study, I would assign “Vital Sign”
• in the future it might be worth adding parameters derived by the derive_param_computed() function to derive, for example, WSTHIPR (Waist to Hip Ratio [%]) or DTG (Distance to Goal (BMI) [%])

Thank @starosto - nothing precludes us from updating the vignette anyway 😄

These parameters sound interesting. are they common in your study? @pharmaverse/admiralmetabolic - have you seen these before?

[starosto]

@manciniedoardo yes, we have these parameters in some studies. DTG is derived as 100*(BMI at baseline - BMI at visit)/(BMI at baseline - target BMI). Target BMI depends on BMI at baseline and should be defined by the user.