Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine admiral Strategy #954

Closed
5 tasks done
bundfussr opened this issue Feb 17, 2022 · 4 comments · Fixed by #1068 or #1114
Closed
5 tasks done

Refine admiral Strategy #954

bundfussr opened this issue Feb 17, 2022 · 4 comments · Fixed by #1068 or #1114
Assignees
Labels
discussion documentation Improvements or additions to documentation

Comments

@bundfussr
Copy link
Collaborator

bundfussr commented Feb 17, 2022

We updated, extended, and removed many admiral functions in the last year and we had a lot of discussions on how flexible functions and parameters should be and on the level of nesting.

To reduce the number of updates and discussions the admiral strategy should be refined.

This helps also new contributors and developers of admiral extension packages.

Proposed roadmap:

@bundfussr bundfussr added documentation Improvements or additions to documentation discussion labels Feb 17, 2022
@rossfarrugia
Copy link
Collaborator

@bundfussr Here goes with trying to summarize my take on the discussions we’ve spoken about and things that could be added to the programming strategy:

  • Readable code – could add more specifics about why we value transparency and simplicity and some general guardrails, e.g. we are trying to ensure that when any user or regulator looks into a function code (if they ever deem needed – as our documentation will also be great!) then they are able to make sense of it. So be aware for example that nesting a function within a function within a function etc may reduce readability. Or coding a function with say >10 different arguments may make the function more difficult to understand and use, plus more difficult to test for the developer.
  • On the other hand we do have computation functions that can be re-used within admiral to save repeating same code across derivation functions, so this is always going to be a balance and there’s not always a clear single way of doing things. Our main ask is to put yourself in the shoes of our user and consider the scope and expectations of admiral shown at https://pharmaverse.github.io/admiral/index.html.
  • It’s ok to group multiple variables by analysis concept without creating individual wrapper functions for each variable – rationale being we don’t want our reference page and codebase to be flooded with near identical programs that would impact findability. Plus people have the ADaM vignettes and templates anyway to help them identify the functions they need.
  • Tidyverse/dplyr usage highly recommended over base R for consistency

@bms63 @teckla-gsk @koegerr @thomas-neitmann @kabis-ops @millerg23 - please add.

There was a lot more context and background which I can't cover all of here and don't think should go in the documentation, but some of the factors we have to consider are the industry-wide scale we're going for with admiral, ensuring user support is scaleable by enabling users to have the confidence to solve their own issues, building trust in admiral from both potential new company adopters and regulators perspective, ...and a million more good points that i didn't take notes for!

@bundfussr
Copy link
Collaborator Author

@rossfarrugia , thanks for the summary.

If we want to mention the term "analysis concept" in our programming strategy, we need to define it. It is not a CDISC term. I think the "analysis concept" from the Roche specs is not useful for our purposes. E.g., there are different analysis concepts for ASTDY and AENDY.

I would avoid mentioning a threshold for the number of arguments because contributors may combine two parameters into one to fulfill the requirement. But this would result in more complex parameters and decrease the usability.

I would like to add the requirement that the purpose of the function, the purpose of each parameter, the permitted values for each parameter, and the output of the function must be clear from the documentation without knowledge of the code. I think this avoids too complex functions and parameters and the black box effect.

Regarding nesting we could add that code should not be nested without reason. Reasons are

  • avoiding duplication of code,
  • structuring of code to improve readability and to simplify testing,
  • modularization of code, i.e., users could use part of the code for their specific derivations.

@rossfarrugia
Copy link
Collaborator

All makes sense to me. Also to the analysis concept point, maybe adding an example would help best explain here. As soon as Thomas mentioned the "study day" example it's so much clearer. Nobody would ever want to see a unique function for every single possible ---DY variable.

@rossfarrugia
Copy link
Collaborator

FYI @bundfussr i'm working through a draft of this to share. I see its really important to agree as we start to move on with package extensions and bringing lots of new developers to the table.

rossfarrugia added a commit that referenced this issue Apr 11, 2022
rossfarrugia added a commit that referenced this issue Apr 11, 2022
rossfarrugia added a commit that referenced this issue Apr 11, 2022
rossfarrugia added a commit that referenced this issue Apr 11, 2022
@rossfarrugia rossfarrugia mentioned this issue Apr 11, 2022
13 tasks
@rossfarrugia rossfarrugia linked a pull request Apr 11, 2022 that will close this issue
13 tasks
rossfarrugia added a commit that referenced this issue Apr 11, 2022
rossfarrugia added a commit that referenced this issue Apr 13, 2022
Co-authored-by: upadhyor <98332728+upadhyor@users.noreply.github.com>
bms63 added a commit that referenced this issue May 5, 2022
@duniek duniek linked a pull request May 6, 2022 that will close this issue
13 tasks
bms63 added a commit that referenced this issue May 6, 2022
Merge remote-tracking branch 'origin/devel' into 954_admiral_strategy
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
bms63 added a commit that referenced this issue May 10, 2022
@bms63 bms63 closed this as completed May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants