Skip to content
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

Random Draws For SEIR Parameters Per Slot #429

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

TimothyWillard
Copy link
Contributor

@TimothyWillard TimothyWillard commented Dec 17, 2024

Describe your changes.

This pull request addresses the bug described in GH-406 where new seir parameters were not drawn across slots.

Does this pull request make any user interface changes? If so please describe.

The user interface changes are:

  • Added a new Parameters.reinitialize_distributions method that recreates the randomly drawn seir parameters so that they do not hold onto the prior random state, and
  • SEIR.run_parallel_SEIR will now draw random seir parameters across jobs.

These changes have not been documented yet, @alsnhll where do you think is the best place to describe this behavior (I'll make these changes before marking this PR as ready for review)?

What does your pull request address? Tag relevant issues.

This pull request addresses GH-406.

Tag relevant team members.

@alsnhll

Created a test that shows the issue of the SEIR parameters not being
randomly drawn per a slot wheras the outcome parameters are. Test
currently fails.
Added a method to reinit distribution parameters. When the `Parameters`
class is initialized it creates this distribution parameters but also
captures the random seed at the time of initialization which makes the
draws non-random across processes. Added a method to reinitialize
distribution parameters when reseeding. Not a perfect solution, the
ambigious random seed settings is rather confusing for development.
Added a unit test to the `Parameters` class unit tests for the
`reinitialize_distributions` method that demonstrates how this method
affects the seeding behavior across workers.
@TimothyWillard TimothyWillard added bug Defects or errors in the code. gempyor Concerns the Python core. high priority High priority. next release Marks a PR as a target to include in the next release. labels Dec 17, 2024
@TimothyWillard TimothyWillard changed the base branch from main to dev December 17, 2024 20:32
@TimothyWillard TimothyWillard marked this pull request as ready for review December 18, 2024 16:33
@alsnhll
Copy link
Collaborator

alsnhll commented Dec 18, 2024

I did some testing of how random draws of parameters work for each parameter type, using the config config_sample_2pop_modifiers_test_random.yml which is now in the tutorials folder

In the original code in the main branch, the behavior is:

  • spar file, seir:parameters:Ro - same value by slot - Not working as intended
  • hpar file, outcomes:incidDeath:delay - varying by slot and by location. Only taking on integar values.
  • snpi file, seir_modifier:modifiers:Ro_relax - varying by slot and by location.
  • hnpi file, outcome_modifiers:modifiers:test_limits - varying by slot and by location.

Thus variation in spar was the only one not working as intended, while variation in hpar, snpi, and hpi seem to be working as intended.

In this branch, the behavior is:

  • spar file, seir:parameters:Ro - varying by slot
  • hpar file, outcomes:incidDeath:delay - varying by slot and by location. Only taking on integar values.
  • snpi file, seir_modifier:modifiers:Ro_relax - varying by slot and by location.
  • hnpi file, outcome_modifiers:modifiers:test_limits - varying by slot and by location.

So now variation in spar seems to be working as intended, and nothing has changed about how hpar, snpi, and hnpi work.

However note that users are likely surprised by the variation by location in hpar, snpi, and hnpi in this case, as I don't think this behavior is documented anywhere. It's likely not clear to most users that these parameters have different values by location by default when they randomly vary, even though when their value is fixed they have the same value by location. Is this what we want the default behavior to be? I know during inference values are fit by location by default but does that make sense as the default for forward simulations? It's also confusing that spar parameters don't vary by location while these other ones do (I know that's a long standing flepimop issue but it re-confuses me every time I encounter it).

If I instead run a config where for both seir and outcome modifiers I use subpop_groups: "all" - config_sample_2pop_modifiers_test_random_subpop_groups.yml- (which should force all subpopulations to have the same parameter value, at least for inference), then I get

  • snpi file, seir_modifier:modifiers:Ro_relax - varying by slot but same across locations.
  • hnpi file, outcome_modifiers:modifiers:test_limits - varying but same across locations.

which is what we expect. I don't know how to make hpar parameters be grouped by location - can we do this?

To summarize, yes this PR seems to fix the main underlying problem, but this issue has led me to notice a behavior that needs to be documented better and discussed - during forward-only simulations, when hpni, snpi, and hpni are set to be drawn randomly from distributions, they very by slot and location, while spar varies only by slot.

Thoughts from @jcblemai, @shauntruelove, @saraloo on the desired behavior here would be helpful.

_test_random has one parameter of each type (seir, outcome, seir_modifiers, outcome_modifiers) that varies by slot
_test_random_subpop_groups is same as above but for snpi and hnpi parameters it uses subpop_groups to force the same for all locations
@pearsonca
Copy link
Contributor

Nice investigation.

Agree we need to clearly document what matches / doesn't when stochastically simulating. Where in the gitbook do you think it's best to capture that?

Regarding behavior, all the stochastic features should match in the same way in my opinion. Weird that spar seems to deviate, so we should figure out why that is. I kind of think that location shouldn't cause a different sample either, but it's most important that we have consistent behavior.

@jcblemai
Copy link
Collaborator

jcblemai commented Dec 19, 2024

I agree Alison on consistency. The current state is not motivated by anything but historical hastly made decisions during COVID (fixed R0 across subpops, but age structure different in each state). We should change that carefully.

Now:

  • @alsnhll I don't see this config in the repository (perhaps I am in the wrong branch), does the outcome section has a parameter modifying file (another weird thing).
  • @TimothyWillard An opinion: While this addresses the issue, I believe the underlying problem is the seed issue (see my comment here [Bug]: Sampling parameters from distributions not working as intended #406 (comment): python subprocesses inherit the random process state), and just resetting the distribution leaves other possible bugs:
    • One would expect seeds to be different for each slot -- we can talk about handling that deterministically for reproducibility (but right now it is not done deterministically even if it is the same) but at least different simulations should not share seed.
    • So in addition to allowing other random generators to still be in sync (e.g if one does stochastic simulations), it is pretty fuzzy (for me though, perhaps you got it) how it would work without being at the mercy of a scipy distribution implementation change (right now created distributions uses their own seed ? what happens here that makes the number different) ?
    • Even if we keep this update distribution approach, we need to seed each correctly with a different seed (at least like the emcee, ideally deterministically).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defects or errors in the code. gempyor Concerns the Python core. high priority High priority. next release Marks a PR as a target to include in the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Sampling parameters from distributions not working as intended
4 participants