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/
9 stars 1 forks source link

Renaming infections_mean_varname to infections_varname #205

Closed gvegayon closed 6 days ago

gvegayon commented 1 week ago

Renames infections_mean_varname to infections_varname in latent.infections and latent.infectionswithfeedback.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.47%. Comparing base (4e707f5) to head (001db38).

Files Patch % Lines
model/src/pyrenew/latent/infections.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #205 +/- ## ========================================== + Coverage 92.39% 92.47% +0.07% ========================================== Files 40 40 Lines 881 877 -4 ========================================== - Hits 814 811 -3 + Misses 67 66 -1 ``` | [Flag](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/205/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/205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | `92.47% <50.00%> (+0.07%)` | :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.

AFg6K7h4fhy2 commented 1 week ago

I think latent_infections_varname would be clearer, to disambiguate from observed_infections.

I made a comment at nearly the same time. Are we sure latent_infections_varname will always be the most fitting name here?

@damonbayer

damonbayer commented 1 week ago

I think latent_infections_varname would be clearer, to disambiguate from observed_infections.

I made a comment at nearly the same time. Are we sure latent_infections_varname will always be the most fitting name here?

@damonbayer

Yes. These are changes in the latent sub-module.

AFg6K7h4fhy2 commented 1 week ago

Ah, yes, I should have remembered more of what's in ./pyrenew/model/.

Given you statement @damonbayer, I also think infections_varname → latent_infections_varname is clearer.

AFg6K7h4fhy2 commented 1 week ago

Now would be a good time to remove the default values for the varnames as well, per #198

I can do this after our 1-1.

gvegayon commented 1 week ago

Just saw the error. Will fix in a couple of hours

damonbayer commented 1 week ago

Upon further inspection, I elected to remove this altogether. We already had a numpyro.deterministic sample for all_latent_infections (which includes both the seed_infections and the post_seed_infections) in the model class, so saving these separately was redundant.

I think there could be an argument for keeping them within the infections module and removing them from the model code.