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

Refactor convolve scanner factory functions #161

Closed dylanhmorris closed 2 weeks ago

dylanhmorris commented 3 weeks ago

Ready for review.

This PR:

Out of scope

codecov[bot] commented 3 weeks ago

Codecov Report

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

Project coverage is 94.78%. Comparing base (48b7471) to head (b5178a6). Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #161 +/- ## ======================================= Coverage 94.77% 94.78% ======================================= Files 39 39 Lines 842 843 +1 ======================================= + Hits 798 799 +1 Misses 44 44 ``` | [Flag](https://app.codecov.io/gh/CDCgov/multisignal-epi-inference/pull/161/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/161/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CDCgov) | `94.78% <100.00%> (+<0.01%)` | :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.

dylanhmorris commented 3 weeks ago

Should we check the size/shape/type of inputs?

@damonbayer are you talking about checking the inputs to the factories or about having the functions they produce/return perform input checks?

damonbayer commented 3 weeks ago

@damonbayer are you talking about checking the inputs to the factories or about having the functions they produce/return perform input checks?

Checking inputs to factories so we can provide useful errors.

dylanhmorris commented 3 weeks ago

Things that occur to me:

  1. Check that the double scanner tuples are of exactly length two. As written, it will error if they are less.
  2. Check that the dists are ArrayLike and the transforms are callable.
  3. Check that the pair of dists are of the same length (in the future, we might want to have the double scanner just use history subsets of the length of the longer array, and automagically sub-subset when taking the dot product with the shorter array, but that feels a bit implicit.

Stricter stuff that I'm more reluctant to implement:

  1. Could try to check that the transforms are shape-preserving for jax arrays, but that's a bit tricky.
  2. Could force the arrays that go into the dot products to be flat.

Anything else you had in mind @damonbayer?

damonbayer commented 3 weeks ago

@dylanhmorris Sounds good to me.