Scope of {admiraldata}

[manciniedoardo]

The initial idea for {admiraldata} was for it to contain/collect all the SDTM test data from the various {admiral packages}. However a good point was raised in a standup that this doesn't quite work in practice, as, for instance, the programs to generate some of the {admiralonco} test data require ADSL . Thus for them to be ran within {admiraldata} requires a copy of ADSL in the package too.

How should {admiraldata} approach this problem? Should we have some ADaM datasets in the package as well? Should we have all ADaM datasets, ie should all the data for all the admiral packages live within {admiraldata} only? Or would this make the package too large?

@pharmaverse/admiral thoughts?

[manciniedoardo]

From a conversation with @rossfarrugia:

Perhaps the best avenue would be to simply provide both SDTMs and ADaMs in {admiraldata}. The ADaMs would be generated by the templates stored in {admiral} and then saved in {admiraldata}'s data folder. It would also be nice for users to be able to load in test ADaMs, as well as test SDTMs simply by loading in {admiraldata}. A couple of considerations:

1. {admiral} used to store ADaMs in the package that were the result of running the templates, but did away with it due to concerns about package size. This would be (somewhat) mitigated by simply storing all data in {admiraldata} instead. However there would still have to be a conscious effort to reduce the size of the ADaMs, perhaps by limiting the number of patients/tests. This is important to not slow down our CICD stuff too much.
2. We would still store template dataset code in {admiral}, but save the output obtained when running those scripts within {admiraldata}. This does create a circular dependency between {admiral} and {admiraldata}, but as the two packages are always published around the same time we don't envision this to be a major problem.
3. However we would need to add some piece of documentation explaining that when users update a template in {admiral}, the corresponding dataset has to be re-generated and saved in {admiraldata}.

@pharmaverse/admiral @pharmaverse/admiraldata what are your thoughts?

[rossfarrugia]

thanks Edoardo, good summary! for point 3 i recommend adding to our PR template for the current checkbox we have around updating the ADaM templates.

[bms63]

I feel like this is a little more tricky.

So we have a template script for ADSL in core, vaccines and others?? For core, the ADSL dataset created via the template script is needed in some examples, vignettes, etc. Assuming this is the case for other packages that either need our ADSL or have a specific ADSL available to them.

Perhaps the template script for admiral::ADSL can be run in admiraldata and the data stored at every admiraldata PR? So...ADSL template scripts are brought in from core and extension packages, fired off and built and the data stored. This way the data in admiraldata stays current with whatever updates we make to the the script over time....but then how does the admiral vignettes or extension vignettes get this new data???

....Not really sure best approach?? But keeping track of updates to the template scripts and then manually moving the ADSL data to admiraldata seems tedious to me and might cause us some headaches.

Might it just be easier to keep ADSLs in their specific packages?

[manciniedoardo]

Might it just be easier to keep ADSLs in their specific packages?

I think if we include some ADaMs in {admiraldata}, then we should include all ADaMs; if we need multiple copies then they can be prefixed with admiral_, admiralonco_, etc

[bms63]

What are the other ADaMs? I thought it was only ADSLs that were needed to be stored?

[manciniedoardo]

the proposition is to store each ADaM for which we have a template

[bundfussr]

What are the other ADaMs? I thought it was only ADSLs that were needed to be stored?

In admiralonco we stored ADRS because it is used for ADTTE.

[bms63]

So anytime a template script (12 in core) is updated in core or extension then we have to update admiraldata with that dataset?

I'm not opposed to this idea, I'm just really worried about things becoming mis-aligned if there is a requirement to manually move the data. Some sort of automated pipeline seems to be needed here.

[manciniedoardo]

Just leaving a comment here recording a voice discussion with @cicdguy who confirmed yesterday that an automated pipeline is possible to set up.

[manciniedoardo]

Maybe the {admiraldata} package mantainer could do a quarterly "rerun" of all template scripts prior to release? So that you capture any updates made during that development increment?

[bms63]

I'm hesitant to embrace anything that requires a manual re-run.

If we are doing a PR of devel into main maybe a CI job could trigger all the templates to be run and then stored into the appropriate folder as the final check.

[millerg23]

I don't understand the rationale for wanting to store ADaM data sets? If we want to keep the new repo {admiraldata} just for SDTMs, we could store ADSL on admiral {core}, as its required for admiral Vignettes (is it just Vignetes??).

[cicdguy]

If the 2 data packages are being split, why not have 2 separate websites?

If you want to unify them, consider using Quarto instead of pkgdown.

[manciniedoardo]

Maybe it's ok to have two websites. I guess the admiralsdtm website would have information about our test data strategy while the admiraladam website could be much more barebones and just have a catalogue of the ADaM datasets we provide in there?

[shajoezhu]

Integration tests are a good idea. However I wonder if we can cover both integration tests and example data for vignettes with one set of datasets. Maybe it is better to separate them. I think for vignettes we need small datasets covering the most common scenarios. For integration tests we need data which covers both common scenarios but also edge cases.

having two sets of data is exactly what we are doing now. The small data helps to maintain the size of the package, and only keep the necessary data for testing and examples.

the more comprehensive data helps us to identify issues when all things are integrated together

[shajoezhu]

@manciniedoardo Thanks for your information. I like the idea of a unified approach on test data, but does it mean NEST team will retire their current data packages random.cdisc.data/scda/scda.2022 and transition the data to admiraldata or admiraladam?
Or are we going to update the current admiral ADaM template, e.g., include the variables/records needed for the downstream TLG Catalog? A more detailed plan on the "unified approach" would help narrow down the scope.

@kaz462 very good point. We have revisited this issue a few times, initially admiral template create data isn't sufficient to cover all our usage. I think potentially in the future, we could take a hybrid approach for using both admiraldata and scda.2022

[shajoezhu]

I like this idea a lot. @kaz462 and @pharmaverse/admiraldata what do you think?My CICD knowledge is limited so I'd have to look into what is actually possible.

@kaz462 @manciniedoardo @millerg23 @bundfussr

Our experience from NEST package is that we find snapshot output is very useful. For development, we snapshot all our templates, and if some upstream code is breaking something it wasn't meant to, it helps to catch (at least flag there is a problem) instantly. that is why I think such package would be very useful for admiral develpment as well. :)

[bundfussr]

It seems that we have three or four use cases for the test data:

• vignettes in admiral packages
• test data for admiral templates
• test data for NEST templates and/or vignettes/documentation (it needs to be clarified for what NEST wants to use the test data)

I wonder if it makes sense to cover all use cases by a single set of datasets. For admiral vignettes about five patients should be sufficient. In this case most values can be created explicitly. For NEST I would expect that 20-50 patients are required. Most likely values for these patients are generated randomly. Creating values randomly such that they are realistic and show the desired scenario is not easy. Why should I spend a lot of time generating data for 50 patients if I need only five for writing a vignette?

Furthermore, how do we avoid that updates for one use case does not spoil another one? For example an update of the data for NEST could change admiral vignettes.

[kaz462]

I agree that we need more clarification on the "unified" data approach with the NEST team - to evaluate if admiraladam is necessary instead of rushing to the ADaM package

[shajoezhu]

dear @kaz462 and @bundfussr , thanks for including us in the thread. I think admiraldata package creation would be very valuable for many of downstream package development work! Massive contribution to the community. Thank you so much for taking this on!

Just to clarify the scope for the NEST team usage, we don't plan to use admiraldata as an upstream dependencies. I think this will make a too strong dependencies between two project team and packages, and will significantly extend release cycles. NEST packages (almost all) have their own minimal data for documenation/vignette/test purposes, as the documentation and vignette are user facing. If I may, I would suggest admiral to keep some very minimal sdtm data in the package as well, and just use them for documentation, examples and tests. but have the lengthy data saved seperately in admiraldata.

To be able to test and demonstrate examples on releastic data is very important, for both development work and the community. I imagine, admiraldata will potentially cover many domains and dataset. and I believe our NEST packages and catalog will use proportion of these data, but being able to use something more releasitic in size and values, it will help greatly downstream, and help anyone to better understand the data more quickly.

Thanks!

[rossfarrugia]

hi team, just added an extra use case that it would be great to have our Pharmaverse examples (https://github.com/pharmaverse/examples) work off this test data so we have e2e examples of different pharmaverse packages working together over the same test data.

I'm really keen here that we take this opportunity to think wider than only admiral and approach it with a one pharmaverse mindset. after all, if it wasn't for NEST team we never would have had @cicdguy and his team who have contributed so much to admiral, so a great chance for us to give back to benefit wider packages.

Having followed all of the discussions above I suggest we create the following:

• {pharmaversesdtm} - create as an updated version of {admiral.test} brought more in line with latest CDISC standards and easy to extend to new TAs, and output 2 sets of test SDTMs - one with all patients and one with a smaller subset (for vignettes usage)
• {pharmaverseadam} - feed both sets of SDTMs through admiral templates to create and store the equivalent 2 sets of test ADaMs (and fine if an ADSL or ADRS needs to also be stored within {admiral} / {admiralonco} as usual for the templates check to run and to avoid a circular dependency)
• New CI integration tests so that everytime an admiral template is updated we see if anything changed in the stored {pharmaverseadam} ADaM - so the user can check this was intended
• New automated pipeline to update {pharmaverseadam} ADaMs for each new release for any templates that changed

The good thing about using {admiral.test} as the starting point is this does use a real anonymised study CDISC made available so we know the test data is realistic, and I think it has sufficient patients and data for all our needs.

@manciniedoardo & @bms63 i'd suggest you guys meet 1:1 though to conclude on all the great discussions above and set the strategic direction for our team now. (and i wont be offended if you ignore my suggestions!)

[manciniedoardo]

Thanks @rossfarrugia for the input. Is there a specific reason you suggest to create two versions of SDTMs, one for testing and one for vignettes, rather than, say, identifying a subset of patients vignette_pts that we can filter any SDTM dataset for before we put it through a vignette?

[rossfarrugia]

not really, just there were a few mentions of preferring 2 sets above. it'd probably be cleaner the way you mention and we do that approach in many of the admiral vignettes currently with {admiral.test} so would be fine by me. different vignettes may even prefer to include different patient subsets dependent on which have the most interesting scenarios for the derivations included.

[kaz462]

@shajoezhu @rossfarrugia Thanks for the inputs and summary. I now have a better idea of the scope/purpose of these data packages.

hi team, just added an extra use case that it would be great to have our Pharmaverse examples (https://github.com/pharmaverse/examples) work off this test data so we have e2e examples of different pharmaverse packages working together over the same test data.

adding to the above point - in RConsortium/submissions-pilot3, there are five ADaM programs available based on CDISC pilot data. we also updated the corresponding ADaM define-xml and went through P21 check, so the spec and programs can be cross-checked by users. Maybe we can add them to e2e examples?

[manciniedoardo]

Decision from the standup meeting discussion: go with @rossfarrugia 's suggestion, with the modification that we store vignette datasets that are smaller than the test datasets within the admiral package. This avoids revere/circular dependencies. We would however make sure not to export the vignette datasets so that end-users don't see them, and only turn to pharmaversesdtm/pharmaverseadam for their data needs.

[bundfussr]

@rossfarrugia , I would not expect that the vignette datasets are copies of the test datasets. In admiralonco we tried the approach of generating data for all patients and then selecting the ~5 most interesting patients. This did not work well and is hard to maintain. For example, if the data is generated randomly, the interesting patients change when the code is updated. You also can not see in the code why the patients are interesting.
For the questionnaires vignette we created the data explicitly by tribble() and mutate() calls. This was much quicker to set up and much easier to maintain.
Thus I think separating vignette and test data is the easier approach.
This does not mean that we need to update all existing vignettes. I would use the new approach for new vignettes. If an existing vignette requires an update, I would decide case by case if we use the existing test data or if we create vignette data.
What do you think?

[rossfarrugia]

So for me the 2 options for vignettes would be either use {pharmaversesdtm} and hide a filter step at the top to select a few patients, or create your own minimal test data using {tibble} and hide that. Either way is fine, but neither way involves the need to store extra .rda files in the {admiral} data folder which I find a redundant step.

However, I don't foresee we need many more vignettes so I think we're probably discussing something low impact in the grand scheme and not worth much extra time spent debating, so I leave @manciniedoardo and @bms63 to go ahead now with making a call and getting this started whichever way they agree best.

[bundfussr]

If we do not store the vignette data as .rda files, we need to add the code which creates the data to the vignette. We did this first when writing the questionnaires vignette but then we decided to move it into a separate file because it simplifies the vignette source file and the data is accessible for users.

Furthermore if the data is created within the vignette it can't be used in other vignettes.

We may not need new vignettes in admiral but I would expect new vignettes in the extension packages. In admiralonco we want to add a new vignette for iRECIST and we need to update the data for the ADRS vignette (It does not have any responders at the moment).

[rossfarrugia]

ok, so its only used in cases where an admiral vignette needs its own specific test data created using {tibble}, i.e. nothing from {pharmaversesdtm} then. my main concern was creating subsetted copies of {pharmaversesdtm} in admiral data folder which seems redundant to me.

[bundfussr]

I agree, if the data is just a subset of pharmaversesdtm data, it should not be stored in the data folder.

[manciniedoardo]

here is one point we missed. At the start of the vignettes we would read in the data by calling dm <- readRDS("data/dm.RDS"). But this call would not work for users

Hmm yeah you're right. In that case we probably can't completely hide the data from users, so I would suggest storing the data in the data folder but labelling it as vignette data to try and avoid too much confusion. Eg have datasets named vignette_dm, vignette_ae etc.

[bundfussr]

I am not sure what the simplest approach is.

We could add onco_rs, onco_tu, and onco_tr to pharmaversesdtm for the RECIST data. These would contain only a few patients and would differ from admiral_rs, admiral_tu, and admiral_tr in admiral.test. For the iRECIST data we could add onco_irecist_rs to pharmaversesdtm.

@manciniedoardo , should we do it this way? And are you sure we won't have to change it again in a few months?

[manciniedoardo]

@manciniedoardo yes let's do it this way. It ensures all our data is in one place.

[kaz462]

I wonder if we can just store one dataset per domain in data folder-

• concatenate the same domain from different use cases, instead of storing separate datasets

  e.g., raw_qs, admiral_qs, vignette_qs, ophtha_qs -> qs

• create user-facing functions that can filter data/select variables, e.g.,

pharmaversesdtm::select_sdtm(
    domain = ,
    purpose = # some pre-defined use cases, e.g., "admiralonco_irecist_rs", "admiral_vignette_adrs"
    ...       # add arguments as needed to provide more flexibility in choosing the subset of data
)

• unit tests to ensure key variables and records are expected for various use cases

[manciniedoardo]

@kaz462 this won't always be possible as TA SDTM datasets can differ not only by parameters present but also variables present. ophthalmology SDTM datasets such as AE or EX have variables xxLAT and xxLOC which aren't required for other TAs.

[bundfussr]

I think creating a single dataset per domain would complicate the maintenance because either we need one long program to create the dataset for all use cases or separate programs (for each use case) would write to the same dataset.
Furthermore, the users would need to know that they have to use select_sdtm() to load the data. If they use data(), they will get an incorrect dataset.

Therefore I would create separate datasets.