search

CDCgov / multisignal-epi-inference

Python package for statistical inference and forecast of epi models using multiple signals
https://cdcgov.github.io/multisignal-epi-inference/
10 stars 1 forks source link

Record deterministic `RandomVariables` by default #148

Closed damonbayer closed 4 weeks ago

damonbayer commented 4 weeks ago

Closes https://github.com/CDCgov/multisignal-epi-inference/issues/127

codecov[bot] commented 4 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.93%. Comparing base (f11ce38) to head (a8b9fe6). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #148 +/- ## ========================================== + Coverage 93.90% 93.93% +0.02% ========================================== Files 36 36 Lines 706 709 +3 ========================================== + Hits 663 666 +3 Misses 43 43 ``` | [Flag](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/148/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/148/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | `93.93% <100.00%> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dylanhmorris commented 4 weeks ago

If record=True is going to be the default (and I think it should be, though I'm persuadable), then I think label should not have a default argument, as this will make it easy for the user to end up with variable name collisions, as occurs in the test suite here:

E       AssertionError: all sites must have unique names but got `a_random_variable` duplicated

Also, since the label attribute will be passed to numpyro.deterministic() as the name argument, I think it should be renamed name for consistency.

https://github.com/CDCgov/multisignal-epi-inference/blob/83dd9ddf11df1b0e5a5059f34fb7e1a84e466b9e/model/src/pyrenew/deterministic/deterministic.py#L17-L29

damonbayer commented 4 weeks ago

@dylanhmorris @gvegayon Ready for review, but expecting many merge conflicts, depending on when other PR's land.