Close (#22) Created Obesity ADVS Vignette #26
Conversation
|
|
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? |
|
@AndersAskeland Not able to resolve the pending two checks . Can you help ? |
|
@Siddhesh2097 I just did a quick check, and it seems to be triggered by using the function 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 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? |
|
While we are waiting to find out how to handle the failing tests, here are some other general comments :)
|
|
@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
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 |
There was a problem hiding this comment.
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!
|
@kathrinflunkert can you also please review? |
There was a problem hiding this comment.
Thanks @Siddhesh2097 - changes look good and i've given it a second pass.
Add PARAMN for all parameters and not just for BMI.
There was a problem hiding this comment.
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.
|
|
||
| ```{r eval=TRUE} | ||
| advs <- derive_var_chg(advs) | ||
|
|
There was a problem hiding this comment.
Remove empty line here
| "TEMP", "TEMP", "Temperature (C)", 9, "Vital Sign", 2 | ||
| ) | ||
|
|
||
| attr(param_lookup$VSTESTCD, "label") <- "Vital Signs Test Short Name" |
There was a problem hiding this comment.
No need to label here
| The following steps are to merge `ADSL` variables with the source data and | ||
| derive the usual `ADVS` analysis variables. Note that only the sections required for this vignette are covered in the following steps. To get a detailed guidance on all the steps, refer the `{admiral}` [Creating a BDS Finding ADaM vignette](https://pharmaverse.github.io/admiral/articles/bds_finding.html). | ||
|
|
||
| Join `ADSL` to your `VS` domain. Only the `ADSL` variables used for derivations are selected at this step. |
There was a problem hiding this comment.
From here to the end of the subsection, I would hide these chunks, remove the explanation and accept the vignette is not completely executable as otherwise we are just duplicating what is in the admiral advs vignette.
| using `{admiral}` functions is the same. We focus on the most common | ||
| endpoints and their derivations mainly found in metabolic trials to | ||
| avoid repeating information and maintaining the same content in two | ||
| places. |
There was a problem hiding this comment.
| places. | |
| places. As such, the code in this vignette is not completely executable; we recommend consulting the ADVS template script to view the full workflow. |
|
@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 😄 |
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
mainbranch until you have checked off each task.styler::style_file()to style R and Rmd filesdevtools::document()so all.Rdfiles in themanfolder and theNAMESPACEfile in the project root are updated appropriatelyNEWS.mdunder the header# admiral<ext> (development version)if the changes pertain to a user-facing function (i.e. it has an@exporttag) or documentation aimed at users (rather than developers)pkgdown::build_site()and check that all affected examples are displayed correctly and that all new functions occur on the Reference page.lintr::lint_package()R CMD checklocally and address all errors and warnings -devtools::check()