insightsengineering / teal.modules.clinical

Provides teal modules for the standard clinical trials outputs
https://insightsengineering.github.io/teal.modules.clinical/
Other
32 stars 16 forks source link

Extract list of choices from the formals of calculation package (e.g. `tern`) into the list of choices in GUI to loose package coupling #525

Open pawelru opened 2 years ago

pawelru commented 2 years ago

Quite often we have copy list of possible values from tern into the GUI element into teal.modules.clinical. Then on each update of tern one needs to also update teal.modules.clinical. This breaks open-close principle by tightly coupling those two packages.

Few examples:

https://github.com/insightsengineering/teal.modules.clinical/blob/825cfc45503a13dbe6aad22e1c92b577782e45c1/R/tm_a_mmrm.R#L666-L672 is a copy of: https://github.com/insightsengineering/tern.mmrm/blob/894cec06b1213f0676c7172a61e056acaef9158f/R/fit_mmrm.R#L132


https://github.com/insightsengineering/teal.modules.clinical/blob/825cfc45503a13dbe6aad22e1c92b577782e45c1/R/tm_g_lineplot.R#L275 is a copy of: https://github.com/insightsengineering/tern/blob/a43dc3a90fc7039d1f9932e78dc93110bbf3369f/R/g_lineplot.R#L138


(there are probably more)


So I think that here (in teal.modules.clinical) we can use choices = eval(formals(tern::foo)$arg)(*) * we need eval because formals() return a pairlist where values are language objects, see below output from my experiments:

r$> formals(tern.mmrm::fit_mmrm)$cor_struct                              
c("unstructured", "xxx")

r$> str(formals(tern.mmrm::fit_mmrm)$cor_struct)                         
 language c("unstructured", "xxx")

r$> eval(formals(tern.mmrm::fit_mmrm)$cor_struct)                        
[1] "unstructured" "xxx"         

If we use above we might loose some coupling and every change in tern won't require changes in tmc. This is a general comment for the overall package.

danielinteractive commented 2 years ago

@pawelru @shajoezhu I think it is a good idea to auto-generate the covariance structure choices available in tm_a_mmrm() from tern.mmrm::fit_mmrm(), if not too complex I would suggest to do this soon

pawelru commented 1 year ago

Even though one PR already went in, let me reopen this as there are probably more places in code with that problem

shajoezhu commented 1 year ago

Hi @pawelru , quick question, I was wondeirng if this issue is currently been worked on, it appears to be on our board

pawelru commented 1 year ago

Hi @pawelru , quick question, I was wondeirng if this issue is currently been worked on, it appears to be on our board

I don't think so.

Melkiades commented 1 year ago

@pawelru I was thinking about this and a solution would be refactoring the way methods and stats are stored in tern, i.e. as a simple character vector getter function. Would this be useful to solve this problem?

pawelru commented 1 year ago

@pawelru I was thinking about this and a solution would be refactoring the way methods and stats are stored in tern, i.e. as a simple character vector getter function. Would this be useful to solve this problem?

Could you please elaborate more and give example? I want to better understand idea before making a statement.

Melkiades commented 1 year ago

Could you please elaborate more and give example? I want to better understand idea before making a statement.

So for example now in tern you have something like: ?tern::count_occurrences where the .stats (character) statistics to select for the table. The default from the "Usage" is .stats = "count_fraction". There are in practice other 2 options that are not apparent. I would create a get_stats or get_method where you can specify the function and type to get the list of available methods/stats. This way, we would have it in only one place, and it would be also possible to retrieve it easily. [function can be "count_occurences" in this case or just the usage group like "counts" (I prefer the second) and type will be like "numeric" or "factor" to see different methods]

Ofc this would have something like get_all_methods/stats for checks (still dependent on function/group of use or type)

pawelru commented 1 year ago

Apologies, I still don't get it completely. Did you mean get_stats(fun = tern::count_occurrences, type = ???)? What is a type argument? What if there are other dot argument like .foo and .bar? How would it know to get .stats and not .foo argument?

Melkiades commented 1 year ago

Apologies, I still don't get it completely. Did you mean get_stats(fun = tern::count_occurrences, type = ???)? What is a type argument? What if there are other dot argument like .foo and .bar? How would it know to get .stats and not .foo argument?

There is a lot of overlap between stats (they refer often to the same groups) but some may change a bit for like formats and so on. In the function parameter of tern::count_occurences it would be: .stats = get_stat("count_occurrences") or better .stats = get_stat("counts") and the function would be like in this draft PR: https://github.com/insightsengineering/tern/pull/1041/files

Take into account only get_stats and getters for labels and formats from that PR, because for the refactoring of count_occurences we have a good workaround atm. Let me know if a design like that would be good, or if we can meet to discuss this :)

Melkiades commented 1 year ago

I think to fix this we should only make apparent in the formals the default for each function in tern. @edelarua @ayogasekaram do you think this is manageable?

edelarua commented 1 year ago

@Melkiades I think this issue is definitely doable, but I don't think teal apps often accept arguments for .stats, etc. so the issue is not as complicated as it could be.

Melkiades commented 12 months ago

@Melkiades I think this issue is definitely doable, but I don't think teal apps often accept arguments for .stats, etc. so the issue is not as complicated as it could be.

I think we can propose this again next increment, as we need a bit of time to check that it is correctly reflected. what do you think?