Skip to content

Climatological #436

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

Merged
merged 60 commits into from
Mar 12, 2025
Merged

Climatological #436

merged 60 commits into from
Mar 12, 2025

Conversation

dajmcdon
Copy link
Contributor

Checklist

Please:

  • Make sure this PR is against "dev", not "main".
  • Request a review from one of the current epipredict main reviewers:
    dajmcdon.
  • Make sure to bump the version number in DESCRIPTION and NEWS.md.
    Always increment the patch version number (the third number), unless you are
    making a release PR from dev to main, in which case increment the minor
    version number (the second number).
  • Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    0.7.2, then write your changes under the 0.8 heading).

Change explanations for reviewer

  1. Adds a canned forecaster that creates "climate" forecasts.
  2. Adds a step to create climate point predictions and (by default) include them as a predictor in a model.

@dajmcdon
Copy link
Contributor Author

dajmcdon commented Feb 12, 2025

@dsweber2 I added the print method and some ugly hacks to get autoplot to work, but I'm not super satisfied by the hackiness. So big-picture ideas there would be appreciated.

Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

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

Generally pretty happy with this, took a bit to understand what was going on, but that's most b/c there's a lot.

I'm making a PR onto this with some of the smaller recommendations and my annotations in the process of understanding.

Backcasting: if we're supporting this, shouldn't we be filtering epi_data to be on or before that date? Otherwise we're using truth data for forecasting

@brookslogan
Copy link
Contributor

brookslogan commented Feb 15, 2025

Just skimming convo on phone, wanted to point out that in past we found holiday effects around weeks 52 (53) 1 were important so to prevent weird stuff from happening from confusing/grouping them we had the concept of season week 31 to 82/83 of season "S2016” aka ”2016/2017" to corresponding to epi weeks 31 to end of 2016 plus 1 to 30 of 2017, or something similar to enable representing entire years. So any weird stuff dropping or conflating whatever would happen in off season where it presumably would not matter, near weeks 31/82/83.

@dajmcdon
Copy link
Contributor Author

dajmcdon commented Feb 15, 2025

The season-week arithmetic is pretty challenging to get right. And I agree that the convention of having "Week 1" in the summer is probably the easiest change. The same problem potentially impacts the leap year + daily case (only worse).

Perhaps a "for now" solution is to (?)

  1. move week 1 to the summer (and document), for both week and epiweek cases.
  2. Daily use 365 as the modulus always, with the effect that Dec 31 will map to Day 1 in leap years.
  3. Have a more careful discussion at some point about how to carefully handle the boundaries.

@dsweber2
Copy link
Contributor

Yeah, throwing the problem into the part of the dataset that's mostly irrelevant seems like a good first pass.

@dajmcdon dajmcdon merged commit 4da9b23 into dev Mar 12, 2025
2 checks passed
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