Open fb-elong opened 2 years ago
In general I agree with these comments. If you ask me for API suggestions:
The first step towards a more functional style and reduce the complexity is to create smaller functions that implements specific methods that are called by this wrapper function. The lower level functions will only need to have named arguments and can avoid ...
. It would also help decouple the many if-else conditions from this one function.
To refactor the high-level wrapper function, consider adding a layer for the method and its parameters because that's the correct level of abstraction for those ...
arguments. For example:
er <- enroll_rates(...)
fr <- fail_rates(...)
# Previously you use
fixed_design(
x = "FH",
alpha = 0.025,
power = 0.9,
enroll_rates = er,
fail_rates = fr,
study_duration = 36,
rho = 1,
gamma = 0
)
# Although you should use
fixed_design(
alpha = 0.025,
power = 0.9,
enroll_rates = er,
fail_rates = fr,
study_duration = 36,
method = fh(rho = 1, gamma = 0)
)
You can see this pattern used by the family
argument in glm()
and glmnet. You may or may not need some sort of non-standard evaluation depending on the design but it should not be complicated.
Does this make any sense?
I love this, particularly the change from x to method, but also to possibly move some computing into an fh()-specific version of fixed_design() to avoid complicating fixed_design().
Each one of the arguments of fixed_design could be a list! Then every combination of those arguments could be put into an output table. The idea is to be able to answer user questions about how design reacts to changes in any argument in a way that is easy to summarize in a table or a plot.
While developing the RMST option for fixed design, here are few suggestions to improve the function.
x
.x
to a more informative argument name. e.g.method
....
....
is not a user friendly approach. How would user know you are expectingweight
,rho
,gamma
etc for different design method? (@nanxstats , any suggestion to refactor?)enrollRates
andfailRates
should be required argument instead of using...
study_duration
instead ofstudyDuration
MC
instead ofMaxCombo
to be consistent with other abbreviation.