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

Remove unneeded abstractions in model code #187

Closed damonbayer closed 1 week ago

damonbayer commented 2 weeks ago

I don't see any reason to have functions in the class that just call one line functions. We might as well call them directly. We actually do this already for some parts of the model, so this is also a more consistent approach.

My overall ambition is to make models as lightweight as possible and encourage users to make models with minimal overhead. Possibly, going as far as to not provide any prebuilt models in the package itself (but provide ample examples in documentation).

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 95.61404% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.39%. Comparing base (f68bea6) to head (5acbd0e). Report is 10 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/latent/infectionswithfeedback.py 66.66% 1 Missing :warning:
model/src/pyrenew/process/periodiceffect.py 96.55% 1 Missing :warning:
model/src/pyrenew/process/rtrandomwalk.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #187 +/- ## ========================================== - Coverage 94.75% 92.39% -2.37% ========================================== Files 39 40 +1 Lines 839 881 +42 ========================================== + Hits 795 814 +19 - Misses 44 67 +23 ``` | [Flag](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/187/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/187/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | `92.39% <95.61%> (-2.37%)` | :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

I like getting lighter code. I would just wait for @dylanhmorris' take on this before merging.

damonbayer commented 2 weeks ago

Noting that I updated the original description for anyone who had previously read it.

damonbayer commented 1 week ago

Once this is merged I think the _rv suffix for those class attribute RandomVariables can be removed, but that's a separate PR.

Happy to discuss further. I know people are generally opposed to variable names that state the variable type, but I think it is an easy remedy to avoid having code like latent_hosp_admissions = latent_hosp_admissions.sample.

damonbayer commented 1 week ago

I have mangled the git history here somehow. Retrying on https://github.com/CDCgov/multisignal-epi-inference/pull/207.