Skip to content

Centralise validation of mvepiestim inputs - #245

Open
sangeetabhatia03 wants to merge 14 commits into
mainfrom
centralise-mvepiestim-inputs
Open

Centralise validation of mvepiestim inputs#245
sangeetabhatia03 wants to merge 14 commits into
mainfrom
centralise-mvepiestim-inputs

Conversation

@sangeetabhatia03

Copy link
Copy Markdown
Collaborator

This PR address issue #189

  • created new helpers to validate inputs to mv-epiestim functions;
  • added an argument to control the validation, set to FALSE for draw_R and draw_epsilon which are called via estimate_advantage
  • created a helper for normalising SI that is called at the beginning of estimate_advantage; our current handling of this was inconsistent as we normalised if the user did not specify t_min and did not normalise if t_min was provided. Further, the code for normalising was also incorrect (fixed now)

@sangeetabhatia03

Copy link
Copy Markdown
Collaborator Author

hi @annecori @steffilazerte just flagging that the lint errors on this PR have been addressed in PR#244.

@steffilazerte steffilazerte left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sangeetabhatia03, I reviewed as best I can, considering my unfamiliarity. Looks good to me and I think has a lot of added value in the DRY principle!

I asked a couple of questions, but mostly just for points of thought, not things that must change (for example, should you add .call = FALSE to the warnings and stops that you move? I'm not sure they'll be delinted otherwise?)

See what you think. Merge when you're ready!

Comment thread R/validate_mv_epiestim_inputs.R Outdated
Comment thread R/gibbs_draws.R
Comment thread R/gibbs_draws.R
Comment thread R/validate_mv_epiestim_inputs.R Outdated
Comment thread R/validate_mv_epiestim_inputs.R Outdated
Comment thread _pkgdown.yml Outdated
Comment thread R/validate_mv_epiestim_inputs.R Outdated
Comment thread R/validate_mv_epiestim_inputs.R Outdated
Comment thread R/validate_mv_epiestim_inputs.R Outdated
Comment thread R/validate_mv_epiestim_inputs.R Outdated

@steffilazerte steffilazerte left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case you wanted a review from me: looks good 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants