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

Adding the periodic and ~~weekly~~ day of the week effects and refactoring #158

Closed gvegayon closed 2 weeks ago

gvegayon commented 3 weeks ago
codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.73%. Comparing base (3d35d13) to head (7b5063e). Report is 4 commits behind head on main.

Files Patch % Lines
model/src/pyrenew/arrayutils.py 96.15% 1 Missing :warning:
model/src/pyrenew/latent/hospitaladmissions.py 91.66% 1 Missing :warning:
model/src/pyrenew/process/periodiceffect.py 96.55% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #158 +/- ## ========================================== - Coverage 94.75% 94.73% -0.02% ========================================== Files 39 40 +1 Lines 839 893 +54 ========================================== + Hits 795 846 +51 - Misses 44 47 +3 ``` | [Flag](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/158/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/158/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | `94.73% <95.83%> (-0.02%)` | :arrow_down: | 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.

gvegayon commented 2 weeks ago

This is now ready for review, @damonbayer.

gvegayon commented 2 weeks ago

At some point, files under docs/source/tutorials were removed unintentionally. 1a75196 fixes that. Also, a figure for testing was added outside of model/. 9b2d667 fixes the latter.

damonbayer commented 2 weeks ago

It's a bit odd that all of the hospitalization code lives in the latent sub-package, because it handles observations as well. Also, the weekday effect is definitely more related to observations. Also, I'm not sure what the process sub-package is supposed to be for. Seems like it has some generic math and some things specific to Rt. Not sure what to make of these observations. Perhaps related to https://github.com/CDCgov/multisignal-epi-inference/issues/163.

gvegayon commented 2 weeks ago

It's a bit odd that all of the hospitalization code lives in the latent sub-package, because it handles observations as well. Also, the weekday effect is definitely more related to observations. Also, I'm not sure what the process sub-package is supposed to be for. Seems like it has some generic math and some things specific to Rt. Not sure what to make of these observations. Perhaps related to #163.

That is too broad of a comment for this PR :). Perhaps submit a few tickets? A couple of answers:

  1. The latent module can handle observations as it is a possibility (optional feat). I don't see any harm in keeping that unless you feel strongly about it.
  2. The way it is built, the day-of-week effect is for a latent variable; how would you observe that?
  3. The process module is for random processes, which have the distinction of having a duration. Perhaps @dylanhmorris can weight in?
damonbayer commented 2 weeks ago

1 and 3 are helpful comments. Thanks.

I don't quite follow 2, but I think I may be misled by the variable names in this PR https://github.com/CDCgov/multisignal-epi-inference/blob/a6cfc9568b038f7ef6f1c97ca715aa469f085f2a/model/src/pyrenew/latent/hospitaladmissions.py#L192-L195

The observed_hosp_admissions actually get passed later to an observation process as a mean parameter, right?

gvegayon commented 2 weeks ago

1 and 3 are helpful comments. Thanks.

I don't quite follow 2, but I think I may be misled by the variable names in this PR

https://github.com/CDCgov/multisignal-epi-inference/blob/a6cfc9568b038f7ef6f1c97ca715aa469f085f2a/model/src/pyrenew/latent/hospitaladmissions.py#L192-L195

The observed_hosp_admissions actually get passed later to an observation process as a mean parameter, right?

100% agree the names are misleading. I've updated them across the functions, tests, and tutorial. It should be ready for a final review now.