pharmaverse / admiral

ADaM in R Asset Library
https://pharmaverse.github.io/admiral
Apache License 2.0
220 stars 61 forks source link

Feature Request from `pharmaverseadam`: add `AVALU` to ADEG #2490

Closed kaz462 closed 2 weeks ago

kaz462 commented 1 month ago

Background Information

https://github.com/pharmaverse/pharmaverseadam/issues/78

Definition of Done

Derive AVALU = EGSTRESU in ADEG template

kaz462 commented 1 month ago

@pharmaverse/admiral @pharmaverse/admiral_comm good first issue

millerg23 commented 1 month ago

I don't think AVALU is a CDISC variable in ADLB, ADVS and ADEG as unit is included in PARAM, up intil now we have only created CDISC derivations in admiral. We originally created them, but then removed AVALU

nbrucken17 commented 1 month ago

AVALU is not a standard CDISC variable, as Gordon noted, because units are expected to be contained in PARAM. However, it has made its way into several FDA Tech Specs and ADNCA, due to regulatory requests or software needs. There has been a proposal to change the variable name to PARAMU for ADaM 3.0, since it’s really an attribute of the parameter itself, not the value, but that’s a few years away. So, my suggestion would be to not routinely include it in ADEG or ADVS, unless it’s really needed for a specific reason.

Thanks, Nancy

kaz462 commented 1 month ago

@millerg23 @nbrucken17 Thank you for the background information! I think it's a good idea to maintain the rule of "only creating CDISC derivations in admiral". In this case, the following variables requested by @edelarua will not be added, @pharmaverse/admiral do you agree?

bms63 commented 1 month ago

Can you add them on in the data package? This is a request from nest correct?

kaz462 commented 1 month ago

Can you add them on in the data package?

The data in pharmaverseadam is generated by running the admiral template

This is a request from nest correct?

Yes, see https://github.com/insightsengineering/scda.test/issues/133

bms63 commented 1 month ago

Could the action in the data package run the template(s) and then add these variables as part of the action?

bundfussr commented 1 month ago

Could the action in the data package run the template(s) and then add these variables as part of the action?

I'm not sure if this is a good idea. If we add extra derivations to data-raw/create_adams_data.R, it could become quite complex. Furthermore, for the users it would be hard to understand how the additional variables are derived. At the moment they can have a look at the template. But if the dataset contains variables which are not in the template, it could confuse the users.

Maybe we should create a separate dataset like adeg_nest which contains the additional variables. The script which creates adeg_nest could read in adeg and add the additional variables. The script could be stored in one of the nest packages. Then we could use the same approach as for the admiral extension packages.

@bms63 , what do you think?

manciniedoardo commented 1 month ago

I tend to agree with @bundfussr - would rather keep the pharmaverseadam action to a simple rerun. Additionally I think adeg_nest is way overkill from us - if NEST needs extra variables for internal Roche purposes then why should we add them to admiral open source? Rather I think they should add those variables in their preprocessing.

bms63 commented 3 weeks ago

I am all for simple!

I guess I thought the purpose of nest using pharmaverseadam was a one-stop shop for the ADaMs they need. So a user could grab the data just from one package that is being used in the Apps.

But yes all for simple. So pass it on to nest. :)

esimms999 commented 3 weeks ago

Ool

Sent from Yahoo Mail for iPhone

On Sunday, August 18, 2024, 8:46 PM, Ben Straub @.***> wrote:

I am all for simple!

I guess I thought the purpose of nest using pharmaverseadam was a one-stop shop for the ADaMs they need. So a user could grab the data just from one package that is being used in the Apps.

But yes all for simple. So pass it on to nest. :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: @.***>

bms63 commented 2 weeks ago

Reflecting on discussion - it seems this issue should be closed. Please re-open if not