search

DARTH-git / dampack

Decision Analytic Modeling (DAM) package is a suite of functions for analyzing and visualizing the health economic outputs of mathematical models
34 stars 9 forks source link

Issue with error checking in OWSA and TWSA functions #151

Closed feralaes closed 3 years ago

feralaes commented 3 years ago

Hi @gknowlt, I recently tried to use the run_dsa function and I noticed that you have a check here for all the elements in the list of params_basecase to be numeric.

https://github.com/DARTH-git/dampack/blob/a033a91a57fa4c6466b694c5ce6a20c998826827/R/run_dsa.R#L63

I think that check shouldn't be there because there might be some elements of the list of parameters that are not numeric (like in the model I'm running).

I suggest changing the code to: if (!all(is.numeric(params_range[, 2]), is.numeric(params_range[, 3]))) { stop("min and max in params_range must be numeric") }

I was able to fix this manually by making a local copy of this function on my computer. However, I encountered another issue that I'm not sure how to fix. I'm getting this error: Error in lapply(c(1:nsamp), wrapper_of_user_model, user_fun = FUN, param_name = pars_i, : object 'wrapper_of_user_model' not found

Is it because you are not exporting wrapper_of_user_model?

Any guidance on how I can fix this will be greatly appreciated.

gknowlt commented 3 years ago

@feralaes Thanks for catching this! You are definitely right that it should not be checking like this. I think the intention was probably to make sure that the basecase parameters for the parameters that are being varied in the DSA (and only those) are numeric.

You're also right that you won't be able to change the function locally and then re-reun the code b/c the wrapper function is not exported. If you changed the code, and then re-built the entire package locally, then this would work.

I'll adjust this code now and let you know when it's fixed. Thanks for bringing this to my attention!

gknowlt commented 3 years ago

Hey @feralaes --I've fixed this issue and put the changes in a new branch called "dev" which I also just published to the Github. I didn't want to put it into master yet because I submitted that version to CRAN and wanted to tag it once it's been accepted. After it's accepted I'll bring the dev version into master.

feralaes commented 3 years ago

Great, thank you for making these changes! To fix the issue with the wrapper_of_user_model function, I loaded it separately, and then the owsa code worked! I will try the dev branch code and will let you know if it all works ok. Best, Fernando