Closed manciniedoardo closed 3 months ago
@pharmaverse/admiral please can you take a look at the new "Programming Concepts and Conventions" vignette and add your feedback here? I'm interested in knowing if you find the content useful and/or if something is missing/superfluous.
Package | Line Rate | Health |
---|---|---|
admiral | 97% | ✔ |
Summary | 97% (4685 / 4813) | ✔ |
Hi @manciniedoardo,
Could you please check for the following words from concepts_conventions.Rmd file ?
I would need to update the WORDLIST for "Walkthrough" word only.
Could you also run the styler for the same concepts_conventions.Rmd file as it currently fails?
Thanks.
Could you also run the styler for the same concepts_conventions.Rmd file as it currently fails?
Thanks.
@Fanny-Gautier thanks, I've done all of this now (I hadn't yet checked the CICD stuff as I'm waiting to get feedback from the rest of the team but I fixed them now)
A good opportunity for @pharmaverse/admiral_comm !! this is for the "Get Started" vignettes and trying to improve the ADaM examples and provide more information on programming concepts and conventions in admiral.
I am not sure if it was a good idea to split the "Get Started" page. To me it looks more confusing than before. For example why is "Derivations", "Input and Output", and "Computations" in the "walkthrough" but "Handling of Missing Values" and "Argument Conventions" in "Programming Concepts and Conventions"?
I believe the intention was to "unbury" the exprs()
stuff from the Get Started page, which we are doing with the split vignette. It is a pretty critical part of programming workflow and from my understanding very unique to admiral. I don't use this in any other packages, but I don't really go beyond the verse packages.
However, I do understand that the content for each vignette might be a bit wonky!!! Let's chat on Wednesday about what to have in each vignette - I'm sure we can reach an accord!! @Fanny-Gautier and @manciniedoardo thanks for breaking up and giving us something to look at more in depth!!
Hi @Fanny-Gautier - did you have time to make the outline in the Issue? I think this will be helpful for us to agree on to minimize back and forth conversation on content.
Hi @Fanny-Gautier - did you have time to make the outline in the Issue? I think this will be helpful for us to agree on to minimize back and forth conversation on content.
Hi @pharmaverse/admiral here is the outline of the 2 vignettes which have been created and updated: Vignette: Simple ADSL & ADVS Walkthrough
Vignette: Programming Concepts and Conventions
Hi @Fanny-Gautier - did you have time to make the outline in the Issue? I think this will be helpful for us to agree on to minimize back and forth conversation on content.
Hi @pharmaverse/admiral here is the outline of the 2 vignettes which have been created and updated: Vignette: Simple ADSL & ADVS Walkthrough
- Main Idea
Can the main idea have some discussion on adding variables versus adding rows and in this vignette we showcase those two concepts with ADSL (adding variable), ADVS (adding records).
- ADSL example o Load Packages and Example Datasets o Derive Treatment Variables (TRT0xP, TRT0xA) o Derive/Impute Numeric Treatment Date/Time and Flag Variables (TRTSDTM, TRTEDTM, TRTSTMF, TRTETMF) o Derive Date Variables from Date/Time Variables (TRTSDT, TRTEDT) o Derive Treatment Duration (TRTDURD) o Derivations
Is this a discussion on general Derivations in admiral? Could we just move it to programming vignette or drop it.
o Input and Output
Can we move to programming vignette
o Computations
I'm a bit unsure if this is really needed to be highlighted anywhere in our Get Started documents?
- ADVS example o Load Packages and Example Datasets o Derive Numeric Date and Analysis Day (ADT, ADY) o Assign PARAMCD, PARAM, PARAMN, PARCAT1 o Derive Results (AVAL, AVALC) o Derive MAP Parameter
- Starting a Script
- Support
- See also
Vignette: Programming Concepts and Conventions
- Introduction
- {admiral} Functions and Options
- Handling of Missing Values
Can we bump to before Common pitfalls
- Argument Conventions
- exprs() o What is exprs()?
- Common pitfalls o 1. Mistakenly passing something that isn’t an expression to an argument o 2. Forgetting that expressions must be evaluable in the dataset
- See also
I'm not sure if this is the right place for this note, and haven't been able to find the ADVS example, but the step regarding derivation of results (AVAL and AVALC) listed above caught my attention. Since the overwhelming majority of vital signs recorded have numeric results, it's unusual to need AVALC in ADVS, since AVALC should only be populated when a parameter is qualitative in nature. In addition, the only time both AVAL and AVALC should be populated on the same record is when there is a code-decode relationship between them, or for qualitative parameters when AVAL is used for controlling the sort order of the responses. This is a very common ADaM implementation issue, so I just wanted to make sure it didn't occur in the examples here. For more information, please see Section 3.3.4.2 of the ADaMIG v1.3 (also present in earlier versions of the ADaMIG).
Thanks, Nancy
I'm not sure if this is the right place for this note, and haven't been able to find the ADVS example, but the step regarding derivation of results (AVAL and AVALC) listed above caught my attention. Since the overwhelming majority of vital signs recorded have numeric results, it's unusual to need AVALC in ADVS, since AVALC should only be populated when a parameter is qualitative in nature. In addition, the only time both AVAL and AVALC should be populated on the same record is when there is a code-decode relationship between them, or for qualitative parameters when AVAL is used for controlling the sort order of the responses. This is a very common ADaM implementation issue, so I just wanted to make sure it didn't occur in the examples here. For more information, please see Section 3.3.4.2 of the ADaMIG v1.3 (also present in earlier versions of the ADaMIG).
Thanks, Nancy
Excellent point Nancy!! We do not follow this in our examples and we should, e.g. https://pharmaverse.github.io/admiral/articles/bds_finding.html#aval @pharmaverse/admiral could someone collect all the places that need an update and place into an issue
On the "Get Started" page I would present the main features/concepts of admiral. For each feature I would just provide a short description and a simple example such that the user gets an idea of it. For details I would link to the reference and user guides.
I would remove the ADSL and ADVS example because it requires too much space and is more focused on ADSL and ADVS concepts than admiral concepts.
What about the following outline for the "Get Started" page?
dataset_add
, by_vars
, new_vars
set_values_to
I would move the "Programming Concepts and Conventions" vignette to the "User Guides". Then it is clear that it must not be read for getting started. It could have the following outline.
expr()
, exprs()
, !!
, and !!!
What do you think?
I found that ADSL and ADVS Walkthrough were redundant with the User Guides - even if they are simplified. I am ok with your approach.
I like this outline a lot!! Thanks @bundfussr.
@Fanny-Gautier when do you think you could have it ready by? We have a release on June 3rd and I think we should review thoroughly as this will be first point of contact for a lot of users!!
Thanks again for driving this forward!!
thanks @bundfussr, @bms63, @Fanny-Gautier for the discussion!
I am happy with the proposed new outline by @bundfussr. I will move the concepts_conventions page under user guides and make the suggested changes. Will try finish by end of next week. Happy for @Fanny-Gautier to cover the Get Started side of the update.
I am leaning towards keeping all tthe work within this PR as we have already made quite a lot of changes in this branch, which will be quite difficult to unravel. Though this will turn into a massive PR 😱 !
@bundfussr I have reworked the concepts and conventions vignette after your through review. please could you take a look at it again? thanks
@bms63 @bundfussr @Fanny-Gautier @Lina2689 as discussed last week, I have completed the revamp according to the discussing in #2395 and this PR, taking over from @Fanny-Gautier and @Lina2689's start. The PR is now ready for review.
@Fanny-Gautier any final comments? Be great to get this merged in before EOD. Let us know if you need some more time.
Done?
unless some sneaky hobbitses notice something else!!
I'm just joking please review to your heart's content - let's make this perfect!😆
unless some sneaky hobbitses notice something else!!
I'm just joking please review to your heart's content - let's make this perfect!😆
I promise I don't look like that in real life 🙈🤣 Trying to complete my review within the next hour 😉
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.styler::style_file()
to style R and Rmd filesinst/cheatsheet/admiral_cheatsheet.pptx
and re-upload a PDF version of it to the same folder.devtools::document()
so all.Rd
files in theman
folder and theNAMESPACE
file in the project root are updated appropriatelyNEWS.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)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 check
locally and address all errors and warnings -devtools::check()