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

Weekly Rt with autoregressive difference #131

Closed gvegayon closed 3 weeks ago

gvegayon commented 1 month ago

This PR adds the following:

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 97.01493% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.29%. Comparing base (394a03d) to head (1af7ba5). Report is 1 commits behind head on main.

Files Patch % Lines
...odel/src/pyrenew/model/rtinfectionsrenewalmodel.py 87.50% 1 Missing :warning:
model/src/pyrenew/process/rtperiodicdiff.py 97.87% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #131 +/- ## ========================================== + Coverage 93.93% 94.29% +0.35% ========================================== Files 36 37 +1 Lines 709 771 +62 ========================================== + Hits 666 727 +61 - Misses 43 44 +1 ``` | [Flag](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/131/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/131/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | `94.29% <97.01%> (+0.35%)` | :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.

gvegayon commented 4 weeks ago

Working on the conflicts

gvegayon commented 4 weeks ago

Ready for review, @dylanhmorris and @damonbayer .

gvegayon commented 3 weeks ago

Looks good, but I had one question about duration. Why did we opt for this instead of n_timepoints? They appear to represent the same concept, but maybe I am missing something.

I am a bit fuzzy about this. The duration keyword is used for processes only, while n_timepoints for models. I don't have strong feelings about it, but duration seemed like a natural name for processes, so I didn't want to change that. But the number of observations in a model didn't sound like duration, so I opted for using n_timepoints. Perhaps this could be homogenized to length or something? I suggest continuing this discussion in an issue.

dylanhmorris commented 3 weeks ago

Happy to standardize on n_timepoints. Probably a separate PR.