search

pharmaverse / admiralonco

Oncology extension package for ADaM in R Asset Library (admiral)
https://pharmaverse.github.io/admiralonco/index.html
Apache License 2.0
31 stars 8 forks source link

Feature Request: Make `source_pd` in `filter_pd()` optional #50

Closed bundfussr closed 1 year ago

bundfussr commented 2 years ago

Feature Idea

Make source_pd in filter_pd() optional and remove logic handling source_pd == NULL in derive_param_*() functions.

Relevant Input

No response

Relevant Output

No response

Reproducible Example/Pseudo Code

No response

rossfarrugia commented 2 years ago

Discussed at team meet today and we agreed to put this one on hold for now and run by testers for feedback later. Generally there's a feeling that making source_pd optional here makes the function too loose, and it then only opens up more questions.

One possible option would be to move filter_pd to admiral core level and name it something more generic and change source_pd argument to something like source_end_date, but the feeling at the moment is some of us quite liked the simplicity of calling this PD and having it onco specific as it resonates with our very common need as onco TA users.

The other point then mentioned is around whether we make clearer in our vignettes that not only PD dates could be used here - e.g. a user could even choose to take the minimum of PD and NACT date.

bundfussr commented 2 years ago

I think the two last items are contradicting each other. If we suggest that source_pd should be used for other dates as well, why do we name it source_pd?

As a user I would be confused and it requires extra work from my side. For example, if I am reviewing an ADRS script where source_pd is set to the minimum of PD and NACT date, I would wonder if the results will be correct. So I need to look at the documentation and code of the derive_param_*() function. I would need to verify that there are no checks verifying that at the specified date PD occurred. I would also need to verify that source_pd is used to restrict the source data only but not for example to identify subjects with PD. I would need to do the same when I am the programmer writing the ADRS script. To be on the safe side I would need to repeat this after each admiralonco release.

rossfarrugia commented 2 years ago

Yes good point. On reflection I actually prefer the approach outlined at https://pharmaverse.github.io/admiralonco/test/articles/adrs.html#input for how to deal with NACT either via ANL01FL or up front pre-processing to filter out records you don't want. Then leave source_pd to just be for PDs. It's much clearer this way. FYI @amitjaingsk

bundfussr commented 1 year ago

The admiralonco functions are superseded by derive_extreme_event(). I would not expect that we develop them further. Thus I am closing this issue.