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

Add weekly_to_daily functionality, move time handling functions to their own module #483

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

dylanhmorris
Copy link
Collaborator

@dylanhmorris dylanhmorris commented Feb 12, 2025

Summary

  • Adds weekly-to-daily value broadcasting functions + documentation and tests
  • Moves timeseries handling to its own module
  • Tweaks how input/output data first day of week handling (see breaking changes)

Breaking changes

This will introduce two breaking changes:
-from pyrenew.convolve import daily_to_weekly will need to become from pyrenew.time import daily_to_weekly.

  • daily_to_weekly now defaults input_data_first_dow to the specified week_start_dow rather than to 0 (Monday) if input_data_first_dow is not specified. I think this is less surprising. The alternative would be to force the user always to specify input_data_first_dow.
    We could go through a deprecation cycle but I think Pyrenew is a bit young for that. @damonbayer / @sbidari lmk if you disagree.

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.67%. Comparing base (5c29559) to head (7a85518).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #483      +/-   ##
==========================================
+ Coverage   96.61%   96.67%   +0.05%     
==========================================
  Files          41       42       +1     
  Lines         976      993      +17     
==========================================
+ Hits          943      960      +17     
  Misses         33       33              
Flag Coverage Δ
unittests 96.67% <100.00%> (+0.05%) ⬆️

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.

Copy link

github-actions bot commented Feb 12, 2025

Thank you for your contribution @damonbayer 🚀! Your github-pages is ready for download 👉 here 👈!
(The artifact expires on 2025-03-07T23:29:53Z. You can re-generate it by re-running the workflow here.)

@sbidari
Copy link
Collaborator

sbidari commented Feb 12, 2025

Why isn't the weekly_values adjusted before repeating to create daily values in weekly_to_daily?

@dylanhmorris
Copy link
Collaborator Author

Why isn't the weekly_values adjusted before repeating to create daily values in weekly_to_daily?

For weekly_to_daily, all weekly values have to be assumed complete. So the key thing is to ensure that any partial starting week is only partially tiled, which the final [offset: ] operation accomplishes. If the user wishes to trim days off the ending week, they must do so manually. I suppose we could add an output_data_last_dow, but I'm not sure whether or not it would make the API clearer. Thoughts?

@dylanhmorris dylanhmorris marked this pull request as ready for review February 26, 2025 19:40
@dylanhmorris dylanhmorris requested a review from sbidari February 26, 2025 19:41
@sbidari
Copy link
Collaborator

sbidari commented Feb 27, 2025

The implementation looks okay to me.
However, I am missing the use case of this weekly_to_daily function. Why are the weekly values converted to daily by simply repeating instead of for example, diving by number of days in a week?

@dylanhmorris
Copy link
Collaborator Author

The implementation looks okay to me. However, I am missing the use case of this weekly_to_daily function. Why are the weekly values converted to daily by simply repeating instead of for example, diving by number of days in a week?

weekly $\mathcal{R}(t)$, for one. This PR is in part motivated by CDCgov/pyrenew-hew#344

Copy link
Collaborator

@sbidari sbidari left a comment

Choose a reason for hiding this comment

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

Thanks @dylanhmorris for clarification, LGTM!

pyrenew/time.py Outdated
if len(daily_values) < 7:
raise ValueError("No complete weekly values available")

weekly_values = jnp.convolve(daily_values, jnp.ones(7), mode="valid")[::7]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
weekly_values = jnp.convolve(daily_values, jnp.ones(7), mode="valid")[::7]
trimmed = daily_values[:len(daily_values) // 7 * 7]
weekly_values = trimmed.reshape(-1, 7).sum(axis=1)

Seems to be about 5x faster

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice, thanks! As written this function only accepts 1D inputs and will error with a greater than 1D input (because jnp.convolve does). The new version also assumes 1D but doesn't verify. We can verify 1D input but even better would be to generalize.

Copy link
Collaborator

@damonbayer damonbayer Feb 27, 2025

Choose a reason for hiding this comment

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

Suggested change
weekly_values = jnp.convolve(daily_values, jnp.ones(7), mode="valid")[::7]
n_weeks = daily_values.shape[0] // 7
trimmed = daily_values[:n_weeks * 7]
weekly_values = trimmed.reshape(n_weeks, 7, *daily_values.shape[1:]).sum(axis=1)

This works, assuming the 0th dimension is "time." It retains the speed benefit, as before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Would you be up for contributing an edit to the tests to check the multi-dim (1st dim time) case?

Copy link
Collaborator

@damonbayer damonbayer Feb 28, 2025

Choose a reason for hiding this comment

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

@dylanhmorris I pushed a test directly to this branch 7a85518 (#483)

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.

3 participants