-
Notifications
You must be signed in to change notification settings - Fork 2
support for placebo groups #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi @yoshidk6 - this is a very early sketch of what the PR might look like for #9. The idea here is that if we want to support scenarios like "user wants to plot the placebo group, but does not want those data being part of the model" then we need to support two new arguments at the modelling level:
When As it stands, all I've done is add these two arguments for the However, it's enough that you can at least see how the approach would play out. The Obviously this is nowhere near ready to merge, but I wanted to run it by you at this stage to see if this general approach makes sense to you? It would be a bit of work to apply this approach consistently across the various model types, but conceptually it seems to make sense to me EDIT: also, agreed with your comments in the issues thread. As it currently stands it will not work with log-transformed exposures, and no attempt has been made to bin the placebo group separately from other cases; that would have to be handled in the plot methods. Happy to take a stab at implementing all this, if we are agreed on the general approach :) |
|
Hi @djnavarro Thanks so much for putting this initial sketch together and starting the conversation! I very much appreciate your proactive check-in on the design choices. Building on your idea, I was wondering what you'd think about consolidating the placebo-related logic into a single, more structured argument. This could make the functionality even more robust and extensible for future use cases. The idea would be to introduce a single list argument, perhaps named ermod_bin(
...,
option_placebo_handling = list(...)
)This list would contain the options for defining and handling the placebo group:
Why this approach could be beneficial:
Example Usage:Scenario 1: Default behavior (placebo is the zero-dose group, excluded from model) fit <- ermod_bin(data = df, var_exposure = "DOSE", var_response = "RESPONSE")Scenario 2: Using an indicator variable (excluded from model) fit <- ermod_bin(
data = df,
var_exposure = "EXPOSURE",
var_response = "RESPONSE",
option_placebo_handling = list(
approach = "var_placebo",
var_placebo = "IS_PBO_GROUP"
)
)Scenario 3: Including the placebo group (defined by zero dose) in the model fit <- ermod_bin(
data = df,
var_exposure = "DOSE",
var_response = "RESPONSE",
option_placebo_handling = list(include_placebo = TRUE)
)This design also reinforces the principle that the What are your thoughts on this alternative structure? Other consideratio
|
|
Oh that's very nice. I like the list argument approach a lot: it nicely addresses the worry I'd had about placebo handling potentially leading to a proliferation of arguments when specifying the model. I'll adapt the PR to use that approach. |
|
Glad you liked it :) It's coming from the approach I took in https://genentech.github.io/BayesERtools/reference/plot_er.html . |
…s placebo options
|
Hi @yoshidk6, My sincere apologies for taking so long on this -- I've been busier than expected, and this turned out to be somewhat trickier than I thought it would. Nevertheless, I've updated PR so that all the Best Key features
The reason for that last feature, rather than dropping/keeping the placebo data at the moment the ER model object is constructed, we leave open the possibility that later on we can allow the user to specify one set of options at the model fitting stage (e.g., ignore placebo when fitting) but override these options at the plotting stage (e.g., display placebo data even though those didn't contribute to the model fit). Actually, that last part would probably be easy to do given the way everything else is structured: I think all that's needed is for Additional features
Limitations
|
|
Thank you so much for the note and truly appreciate all of the efforts you have put in so far! I must admit I underestimated the extent of changes we needed to make. I want to take a bit more time exploring the code & putting together thoughts on this, but wanted to let you know I haven't forgotten about it. Thanks! |
|
no rush. it's been nice to spend time thinking about this one, and I completely agree that it is a surprisingly complicated PR - I wasn't anticipating the need to do a deep dive into the data structures in the package, tbh! in retrospect it does make sense though, because placebo data has a strange relationship to ER analyses. happy to make whatever changes you think might be needed, but as i say... no rush :) |
|
Hi Danielle, thank you very much again for the pull request update! I've been thinking a lot about it for a week and now I'm thinking it might be actually more confusing to users than helpful to have the placebo handling integrated into the model development functionalities. Instead of having these being a part of the I know this is what I proposed to do and you spend a lot of time thinking about and implementing the changes into the codebase, so I feel really sorry about not getting to this thoughts earlier. I'd also love to hear your opinions about it, please let me know your thoughts. |
The intended scope of this PR (once completed) is to allow users to specify placebo groups when building ER models, make decisions about whether placebo group cases should be included in the ER model, and support plots that are able to show (or remove) the placebo group responses irrespective of whether the model was built from those data.
Currently early draft stage, additional detail in comments
closes #9