Skip to content

Conversation

@cdc-mitzimorris
Copy link
Contributor

Changes to pyernew_hew_data.py to use helper functions in PyRenew's time.py where possible.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors pyrenew_hew_data.py to use utility functions from PyRenew's time.py module instead of maintaining duplicate implementations locally. This improves code maintainability by leveraging shared library functions for common date/time operations.

Key Changes:

  • Replaced local get_n_data_days(), get_end_date(), and validation logic with PyRenew's get_n_data_days(), get_end_date(), validate_mmwr_dates(), and create_date_time_spine() functions
  • Updated parameter naming from n_datapoints to n_points to match PyRenew's API conventions
  • Updated test error message pattern to match the PyRenew validation function's error message

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pyrenew_hew/pyrenew_hew_data.py Removed duplicate date/time utility methods (~48 lines) and replaced their usages with PyRenew imports, simplifying the codebase and ensuring consistency with PyRenew's API
tests/test_pyrenew_hew_data.py Updated error message regex from "Saturdays.*MMWR epiweek" to "MMWR dates must be Saturdays" to match PyRenew's validate_mmwr_dates() error message

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.88%. Comparing base (758d34b) to head (c16753c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #742      +/-   ##
==========================================
- Coverage   26.28%   25.88%   -0.41%     
==========================================
  Files          26       26              
  Lines        2427     2411      -16     
==========================================
- Hits          638      624      -14     
+ Misses       1789     1787       -2     
Flag Coverage Δ
hewr 29.93% <ø> (ø)
pipelines 3.05% <ø> (ø)
pyrenew_hew 63.38% <100.00%> (-0.69%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

I don't think the time functions are being exposed in pyrenew.

@cdc-mitzimorris
Copy link
Contributor Author

I don't think the time functions are being exposed in pyrenew.

the problem is here:

pyrenew = { git = "https://github.com/CDCgov/PyRenew", rev = "v0.1.5" }

why did this get pinned to "v0.1.5" and should it be unpinned?

@damonbayer
Copy link
Collaborator

why did this get pinned to "v0.1.5"

#713

and should it be unpinned?

I think you could release v0.1.6 and pin that. I'm not totally against unpinning either, as the pyrenew entry in the lock file isn't automatically updated.

@cdc-mitzimorris
Copy link
Contributor Author

why did this get pinned to "v0.1.5"

#713

and should it be unpinned?

I think you could release v0.1.6 and pin that. I'm not totally against unpinning either, as the pyrenew entry in the lock file isn't automatically updated.

I drafted a release "v0.1.6" @damonbayer does this look OK?

@damonbayer
Copy link
Collaborator

I drafted a release "v0.1.6" @damonbayer does this look OK?

I did one more commit to increment the version number CDCgov/PyRenew#626 and released it. Thanks!

@damonbayer damonbayer enabled auto-merge (squash) November 25, 2025 18:08
@damonbayer damonbayer merged commit 06ba94b into main Nov 25, 2025
66 checks passed
@damonbayer damonbayer deleted the mem_time_redo_2 branch November 25, 2025 18:11
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