2.0 Roadmap (or 1.1)

[bms63]

Rough notes on what admiral 2.0 or 1.1 might look like

These are just musings for discussion - throw in here so we don't lose track of potential work. @pharmaverse/admiral

Check out some of our musings and ideas for 2024 community team!! Feel free to add or comment @pharmaverse/admiral_comm

[bms63]

Tidyselect Implementation

[bms63]

ah that is very interesting - I just read up on that difference https://www.tidyverse.org/blog/2023/04/base-vs-magrittr-pipe/

[manciniedoardo]

Due to the above, I would propose to not go through with this change for the moment.

[bms63]

Going to close as we are not going to pursue

[manciniedoardo]

Call stores

Call stores as described here

[kaz462]

I wonder if we could connect call store with admiraldiscovery and metacore/metatools to make ADaM programming more metadata-driven?

1. Enhance admiraldiscovry to be a centralized metadata that stores functions for each variable/parameter, similar to the purpose of searchability as @bundfussr mentioned here:

If they want to understand a function, they would look into the function documentation. If they are searching code for a particular variable or parameter, they look into the call stores.

• Focus on variables that can be derived with a single function; these variables can then be automated together using use_call(dataset, variables).
• For variables that involve multiple functions during derivation, it might be clearer for programmers to write them in scripts, with admiraldiscovry serving as a reference.

2. metacore: Enhance the metacore object by adding more information, such as a function column to the metacore$var_spec dataset.
   Users can obtain this information by merging their specification with the 'centralized metadata' from admiraldiscovry.

3. metatools: Add a new function ( use_call) to this package that derives a series variables based on metadata.

Not sure if the above aligns the idea of call store though, what do you all think?

[bundfussr]

I wonder if we could connect call store with admiraldiscovery and metacore/metatools to make ADaM programming more metadata-driven?

1. Enhance admiraldiscovry to be a centralized metadata that stores functions for each variable/parameter, similar to the purpose of searchability as @bundfussr mentioned here:

If they want to understand a function, they would look into the function documentation. If they are searching code for a particular variable or parameter, they look into the call stores.

• Focus on variables that can be derived with a single function; these variables can then be automated together using use_call(dataset, variables).
• For variables that involve multiple functions during derivation, it might be clearer for programmers to write them in scripts, with admiraldiscovry serving as a reference.

Once call stores are implemented admiraldiscovery could provide a call store with the "global" derivations and use it to create the webpage. However, I would expect that the majority of derivations are stored in call stores on company, project, and study level. Users would then generate a page like the admiraldiscovery page which combines the calls from all call stores they want to consider.

2. metacore: Enhance the metacore object by adding more information, such as a function column to the metacore$var_spec dataset.

I agree that for a full automation from specs to ADaM dataset we need an enhanced specification format. I am not sure if metacore can cover this. At the moment it is based on define-XML. But this is intended for reviewer and not for automation. For example the order of the derivations is not included and there is no structure for variables which require more than one derivation like DTYPE.

I think first we would need to define a specs structure which allows automation. This structure should be independent of the programming language. Ideally it should also allow to generate the define-XML and it should cover both study specs and standard specs (in standard specs there can be more than one option for a derivation of a variable/parameter). Thereafter we can extend metacore or create a new package to read the structure and create an R object.

Users can obtain this information by merging their specification with the 'centralized metadata' from admiraldiscovry.
3. metatools: Add a new function ( use_call) to this package that derives a series variables based on metadata.

I think use_call() should be part of the call store package. Once we have the specs structure mentioned above we could add a function to metatools (or in a new package) which generates a script of use_call() calls.

[kaz462]

I have created two issues, feel free to comment and look forward to further discussion on this topic!
atorus-research/metacore#99
pharmaverse/metatools#56

[manciniedoardo]

Using ML to predict admiral function calls

Using ML to predict admiral function calls as described here

[manciniedoardo]

Behaviour with large datasets

See here, where derive_vars_joined() failed with a large dataset

[manciniedoardo]

Remove renv?

[esimms999-gsk]

Wow. This is huge since renv is so embedded with three different profiles, etc. I am very interested in this and would like to be involved with the testing.

The Docker image for admiral (used in Codespaces and may also be used by spinning up a container on a Docker Engine [this is how I work]), created in admiralci, has all of the packages/versions pre-installed. My understanding is this is based on the renv.lock info, so will need to be considered if moving away from renv. @galachad @dgrassellyb can you confirm?

[dgrassellyb]

We recently added a devel image (https://github.com/orgs/pharmaverse/packages?repo_name=admiralci) - based on rocker/rstudio:devel - this one is not dependent from renv, and dependencies are installed directly from DEPENDENCIES - but for now we use it only to add an other r-cmd-check job. (it is not used for now for the other github workflows).

But @bundfussr raised a good question about this actually on slack (local package versions can be different from those in the workflows) - don't know exactly how to handle this, a way could be working as you do @esimms999-gsk (using directly the docker devel image for daily development .. but it would completely change the way of working, not sure everyone want this ..!) @cicdguy what are your thoughts on this ?

And just another topic, I guess we will also need to add codespaces devel configuration as well !

[bundfussr]

What about creating an renv.lock from the devel image and provide it to the users (like we do for the other images)?

[cicdguy]

using directly the docker devel image for daily development

I'd recommend using the release version of R instead of the devel version of R for active development. The latter is something you'd use for testing unreleased features of R, while you'd want to use the former to actually match what CRAN is using in order to maintain compatibility with CRAN's package cohort (and version, of course).

What about creating an renv.lock from the devel image and provide it to the users (like we do for the other images)?

That would get stale really quickly given that the devel version of R will always be a moving target. If anything, you would not want to lock in to a point-in-time cohort of packages from an unreleased version of R.

[dgrassellyb]

@cicdguy maybe then we should also add a check on image based on rocker/rstudio:latest then ? For now we only have devel

[manciniedoardo]

admiral's behaviour with regards to retaining labels

See here for an example where variable labels are dropped. Should admiral always retain all labels?

[manciniedoardo]

@kaz462 is that an ADaM-compliant derivation? to me it feels a little weird to base a derivation, which affects individual records in the dataset, on the variable labels, which differ across columns not records.

[bundfussr]

Should we clarify it in admiral documentation: labels may not be retained after using admiral functions?

Yes, I would add it to the Input and Output section on the "Get Started" page.

[kaz462]

@kaz462 is that an ADaM-compliant derivation?

it's a Bayer-specific variable derived in ADaM

[bms63]

I;m sort of intrigued by this - when you have time @kaz462 could you make us a reproducible example??

[kaz462]

here is an example of concatenating labels to derive ADAE.AESREAS

[bundfussr]

Improve "See also"

I would propose to change the usage of the @family tag. At the moment it replicates the section of the "Reference" page. Therefore you can't leave the section by the links provided in the "See also" section.

I would use more than one @family tag. For instance, for derive_summary_records() we could use @family derv_obs and @family derv_sum. The first one lists all derivations adding observations and the second one lists all derivations which summarise like derive_summary_records() and derive_var_merged_summary().

[bms63]

The reference page should have small explanations for the advance functions and when you might use the

[bms63]

Tacking on some outstanding items from previous Documentation Discussion - #2085

Most of these have been addressed, but some were not implemented as they needed further discussion.

[bms63]

The vignettes load soooooooooooooooooooooo slooooooooooooooooooooooooooooooowwwwwwwwwwwwww!!!!!!

[bundfussr]

Make suppress_warning() available for users

Should we move suppress_warning() from admiraldev to admiral and add an option for printing the suppressed warning and add an classes argument?

[bms63]

That sounds good to me! - I think this should be part of the general move to deprecate admiraldev and make the package into a dev website.

[bundfussr]

suppress_warning() should be on the user website. At Roche there are a lot of discussions how to handle warnings. suppress_warning() allows to suppress specific warnings while suppressWarnings() suppress all warnings. Thus I would recommend the former.

[bms63]

do you want to bring it over now rather than later?

[bundfussr]

I think it is OK to do it next year. Users could use it even now if they use admiraldev. If we have it in admiral, it would be just easier to find for users.

[manciniedoardo]

Happy to do this move now, regardless of if/when we move admiraldev back into admiral. That way if the latter is delayed, at least suppress_warning() is still moved. What do you think?

[manciniedoardo]

Now in an issue 😄 (superseding not deprecating)

[manciniedoardo]

Now in an issue 😄

[StefanThoma]

Log

When I ran an admiral workshop last week some participants were concerned with the amount of feedback a end user sees when running a function.
This reminded me of the tidylog package which tells users exactly how tidy-functions affect the dataset being worked on.
I think this would be great to implement in admiral, maybe with a global option to set whether you want this or not.
These outputs would then be captured in the log file as well.

https://github.com/elbersb/tidylog

Here is an example:

joined <- left_join(nycflights13::flights, nycflights13::weather,
    by = c("year", "month", "day", "origin", "hour", "time_hour"))
#> left_join: added 9 columns (temp, dewp, humid, wind_dir, wind_speed, …)
#>            > rows only in nycflights13::flights    1,556
#>            > rows only in nycflights13::weather (  6,737)
#>            > matched rows                        335,220
#>            >                                    =========
#>            > rows total                          336,776

[bms63]

tidylog re-works all the dplyr functions to allow this type of messaging. a lot of our functions use dplyr functions on the backend. i don't think we should introduce tidylog into admiral, but we could look into this providing some better messaging...but I do think this is going to be a lot of work to incorporate.

[zdz2101]

Cyclocomp lintr

We have 3 functions: derive_vars_dt() , derive_vars_dtm() and derive_vars_query() that seem to use a little too much if logic, should re-evaluate how to cleanup the complexity of these functions

[manciniedoardo]

Now in an issue 😄

[bms63]

Rethink the Readme

The Manifesto is really cool, but I think it should be an infographic with a link to the actual manifesto. Some folks from a JApenese Phuse came up with a cool one that we could modify.

[kaz462]

Grading Criteria Metadata NCI-CTCAEv5

1. From lab_grading/assumptions-made-when-grading:

   For BASE to be abnormal we compare it with the Upper Limit of Normal (ULN) ANRHI, i.e. BASE > ANRHI

   • I encountered a situation where ANRHI differs within a subject and lab test due to data from different lab vendors. Should we consider changing the use of ANRHI to BNRIND when determining whether the baseline is normal/abnormal?

2. Some CTCAE grade definitions contain two sets of units, but only one is provided in the current grading metadata. For example, TERM=Hypoalbuminemia has only one row for unit = g/L, so it's not be able to grade for unit = g/dL
   
   can we add more rows to the metadata to cover different units?

[manciniedoardo]

tagging @millerg23 for awareness

[millerg23]

I think both suggestions should be implemented.
For 1. we didn't use BNRIND initially, as it added another dependency (ie you have to create BNRIND to get a grade). However, I think the example @kaz462 gives above is valid, and we should update. We have a column for checking required vars so would alert users if they need to create BNRIND
For 2. we did discuss implementing the US/CV units, but due to time constraints we concentrated our efforts on getting the SI units covered. Now we have a fairly stable state for grading criteria I think it would be a good time to include the US/CV units, most grades compare with ULN/LLN, so shouldn't be a huge effort to add them in for all 3 grading criteria we have implemented. We had a request in Roche to use US/CV units for a lab test, and I manipulated the metadata to do so. So there is a need here.

If rest of the team agree to the updates, feel free to assign me to these, unless anyone else has a burning desire to make the updates.

[nbrucken17]

Just a note- it might not be valid to compare lab data from different vendors within a subject. If the normal ranges are different, that could indicate different assays or measurement techniques were used by each lab, and changes or shifts from baseline could be misleading, since they could represent a difference between labs, rather than an actual change for the subject. In that case, I'd expect to see different PARAM/PARAMCD values for each of the tests.

My 2 cents' worth,
Nancy

[manciniedoardo]

Thanks Nancy for your comment.

For suggestion number 1, my line of thought is that if this could be a requirement for some teams then it would be nice to implement - maybe with a message triggered that warning that you are comparing lab data from two different vendors? Just so that any programmer who is doing this is aware of what they are doing? Not sure if this is feasible, just musing.

For suggestion number 2, it would be good to add this now that we are over the 1.0 hill.

[bms63]

Including other Pharma Companies

I think we should make a concerted effort to involve other Pharma companies in the long-term maintenance of admiral. Getting someone to attend our bi-weekly/monthly meetings from companies like Novartis, Pfizer, Novo-Nordisk could be helpful with exposure to wider audience, maintenance, fresh eyes and new ideas.

[bms63]

Revisit deprecation and superseded strategy

• Do superseded functions have a message recommending another function?
• Do we continue to follow the same 3-stage process for deprecation?
• Using lifecycle tags

[rossfarrugia]

Would be helpful to get a console message for superseded functions, not an error or anything but just a courtesy note to explain what superseded means in our context and that user may consider using xxx instead.

[manciniedoardo]

@rossfarrugia i've mentioned this here so that we can action in 1.1.

[bms63]

Large merges/join can be slow

Big merges/joins of datasets can really take a while. We should look at how we can improve the backend of admiral. Here is an example of admiral running slow - #2224

[bms63]

Do we need a Glossary/Definition section

Discussion around the use of derivation and other words used throughout admiral. #2021

[bundfussr]

Review "Reference" Page

The "Reference" page requires a clean up. For example:

• derive_var_trtdurd() is listed in "All ADaMs" section although TRTDURD is an ADSL variable.
• derive_basetype_records() is listed in "Derivations for Adding Variables" but it adds records.
• The "Derivations for Adding Parameters/Records" has subsections "BDS-specific" and "TTE-specific". There is only one function in "TTE-specific" and all other functions are in "BDS-specific".

[bms63]

Close out Documentation/Rework activities

We need to review this Discussion Item: #2010 (reply in thread) and the Document that Stefan put together around Documentation and Re-work. A lot of the items were closed out, but some were tabled and we should identify and decide what to do with those tabled items.

[bundfussr]

Make name of functions and source objects consistent

• Agree on consistent naming of functions and source objects (for a proposal see admiral Functions Overview).
• Agree on a strategy when and how the renaming should happen.