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

Pyrenew tutorial and tests review #191

Open sbidari opened 2 weeks ago

sbidari commented 2 weeks ago

Some ideas for improving the documentation

  1. Introduction to renewal model framework and some of the model components implemented in the package. The model equations/information from here and here can perhaps be moved to the documentation
  2. Include installation instructions in the beginning of the documentation either in a separate section or within the getting started section.
  3. I am not entirely sure what is the difference between the HospitalAdmissionsModel in Pyrenew demo and the one in Fitting a Hospital Admissions-only model. Perhaps clarify what the two tutorials are trying to convey.
  4. If I understand correctly there are two different model implementation - Hospital Admissions and Reproduction Number Renewal Infections. But it isn't clear from the tutorial, suggesting reorganizing the tutorials to make these two model instances examples clear.
  5. Change the order of tutorial and reference in the documentation

I can make most of these changes (might need some help on 1) if we think these would be useful.

codecov[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 94.64%. Comparing base (81c49f1) to head (ba5a997).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #191 +/- ## ======================================= Coverage 94.64% 94.64% ======================================= Files 39 39 Lines 840 840 ======================================= Hits 795 795 Misses 45 45 ``` | [Flag](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/191/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/191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | `94.64% <ø> (ø)` | | 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.

damonbayer commented 1 week ago

Some ideas for improving the documentation

  1. This sounds good. Please let us know what specifically you want help with.
  2. Include installation instructions in the beginning of the documentation either in a separate section or within the getting started section.
  3. I am not entirely sure what is the difference between the HospitalAdmissionsModel in Pyrenew demo and the one in Fitting a Hospital Admissions-only model. Perhaps clarify what the two tutorials are trying to convey.
  4. If I understand correctly there are two different model implementation - Hospital Admissions and Reproduction Number Renewal Infections. But it isn't clear from the tutorial, suggesting reorganizing the tutorials to make these two model instances examples clear.
  5. Change the order of tutorial and reference in the documentation

I can make most of these changes (might need some help on 1) if we think these would be useful.

  1. This will be covered (at least partially) by https://github.com/CDCgov/multisignal-epi-inference/pull/197
  2. I think installation instructions can go in both the main website page and the getting started tutorial. Note that installation instructions are also here, which is not on the website: https://github.com/CDCgov/multisignal-epi-inference/tree/main/model#readme. This may need to be updated.
  3. Agreed that these are confusing/redundant. I would lean toward merging the PyRenew Demo and Fitting a Hospital Admissions-only Model tutorials. Probably by keeping Fitting a Hospital Admissions-only Model mostly as is, but bringing over anything useful from PyRenew Demo that it is missing.
  4. That's correct. I agree making this more obvious would be useful. We provide two pre-built models: RtInfectionsRenewalModel and HospitalAdmissionsModel. HospitalAdmissionsModel uses RtInfectionsRenewalModel and builds upon it.
  5. Sounds good.
sbidari commented 1 week ago
  1. That's correct. I agree making this more obvious would be useful. We provide two pre-built models: RtInfectionsRenewalModel and HospitalAdmissionsModel. HospitalAdmissionsModel uses RtInfectionsRenewalModel and builds upon it.

I liked the diagram here: https://github.com/CDCgov/multisignal-epi-inference/tree/main/model#readme and can be effective visualization to show HospitalAdmissionsModel uses RtInfectionsRenewalModel and builds upon it.

damonbayer commented 2 days ago

@sbidari Would it be appropriate to also address these comments from @cherz4 in this PR? If not, please create additional issues related to the points discussed on Teams.