mac-theobio / McMasterPandemic

SEIR+ model
GNU General Public License v3.0
20 stars 5 forks source link

Consider adding feature for absolute parameter values in `params_timevar` #135

Closed papsti closed 2 years ago

papsti commented 2 years ago

I know this was discussed before and nixed for some reason (maybe partially because there is a workaround in the current version of the software), but it could be good to enable an option where absolute parameters can be passed via params_timevar... In the last few weeks, two different people (one of whom has worked on Macpan for a very long time!) have been confused about whether some time-varying parameters are specified as relative or absolute values, because in some case, the relative specification is somewhat unnatural: e.g., if we're specifying a new transmission advantage for Omicron vs Delta, this would have to be written as relative to the Delta over Alpha advantage (which is what is stored in the base parameters). When making forecasts and verifying the projection scenarios, it's much easier at a glance to process changes in some parameters if written as absolute values.

A proposed fix would be to allow for a new value in the Type column of params_timevar: add abs in addition to rel_orig and rel_prev. Another idea would be to not change internals, but rather write a method that takes params_timevar and base_params as input, and then outputs a table with the time-varying parameters translated to absolute terms.

@bbolker, do you remember why we originally backed off of specifying time-varying parameters in absolute terms? Would be good to just record it here in case we're tempted to revisit this discussion yet again in the future...

bbolker commented 2 years ago

I don't remember. adding abs as an option seems fairly straightforward. It seems as though adding another clause here would be quite easy? (i.e. something like if (startsWith(type, "rel")) { ... old_stuff ... } else if (type == "abs") params[[s]] <- v)

stevencarlislewalker commented 2 years ago

This option would be trivial to add to the TMB engine

bbolker commented 2 years ago

Is it safe/advisable to merge the TMB branch back into master/main at this point? I'd like to go ahead and add this but hesitated because I don't know what branch to do it on ... (I guess master ...)

stevencarlislewalker commented 2 years ago

I would do it on master. I plan to merge master into tmb 'soon' to make sure nothing breaks. At that point I can add absolute time-varying parameters to tmb. One reason I haven't merged master into tmb is that master is failing several tests.

bbolker commented 2 years ago

added, doesn't obviously break anything (we have some unrelated test warnings about things possibly not working right with testify)

stevencarlislewalker commented 2 years ago

Fixed with completion of #136