Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test convolve mode in hospitaladmissionspy #398

Merged
merged 25 commits into from
Aug 26, 2024

Conversation

sbidari
Copy link
Collaborator

@sbidari sbidari commented Aug 20, 2024

changes included:

  • create a helper function for convolve compute_incidence_observed_with_delay
  • adds test for the helper function
  • update hospital admissions tutorial and DOW tutorials to use convolve mode="valid"

closes #385

@sbidari sbidari linked an issue Aug 20, 2024 that may be closed by this pull request
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.45%. Comparing base (769712b) to head (3efcfad).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   93.43%   93.45%   +0.02%     
==========================================
  Files          38       38              
  Lines        1005     1009       +4     
==========================================
+ Hits          939      943       +4     
  Misses         66       66              
Flag Coverage Δ
unittests 93.45% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sbidari sbidari marked this pull request as ready for review August 21, 2024 23:41
Copy link
Collaborator

@damonbayer damonbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the tutorials, I suggest computing something like n_initialization_points = max(gen_int_array.size, inf_hosp_int_array.size) and using that throughout.

@dylanhmorris
Copy link
Collaborator

dylanhmorris commented Aug 22, 2024

@sbidari can you resolve the conflicts here? Then I will re-review.

…orrect-convolve-mode-in-hospitaladmissionspy
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @sbidari. A few minor tweaks to docstrings and one major suggestion of function placement within the codebase. CC @damonbayer

pyrenew/metaclass.py Outdated Show resolved Hide resolved
pyrenew/metaclass.py Outdated Show resolved Hide resolved
pyrenew/metaclass.py Outdated Show resolved Hide resolved
pyrenew/metaclass.py Outdated Show resolved Hide resolved
test/test_model_hosp_admissions.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@damonbayer damonbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbidari Now that we made a decision around #410, can you adopt those changes for the new names in this PR?

pyrenew/convolve.py Outdated Show resolved Hide resolved
pyrenew/convolve.py Outdated Show resolved Hide resolved
pyrenew/convolve.py Outdated Show resolved Hide resolved
@sbidari
Copy link
Collaborator Author

sbidari commented Aug 23, 2024

I realized n_initialization_points needed to be max(gen_int_array.size, inf_hosp_int_array.size) - 1 instead of max(gen_int_array.size, inf_hosp_int_array.size) and made that change in 5c4395f

@damonbayer damonbayer self-requested a review August 26, 2024 15:04
pyrenew/convolve.py Outdated Show resolved Hide resolved
pyrenew/convolve.py Outdated Show resolved Hide resolved
@damonbayer damonbayer merged commit 1a86104 into main Aug 26, 2024
8 checks passed
@damonbayer damonbayer deleted the 385-incorrect-convolve-mode-in-hospitaladmissionspy branch August 26, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect convolve mode in hospitaladmissions.py
3 participants