-
Notifications
You must be signed in to change notification settings - Fork 8
feat: introduce epix_as_of_current() for convenience #645
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a skim. I should probably look more carefully + actually read the tests; going to stop here & self-re-review-request.
07f9472
to
77b265b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and nearly complete.
- But @dsweber2's question/comment today makes me think we need to either:
- Go ahead and place a check here that "now" isn't too far from the pre-existing version end and/or max.
- Not do the current-time/version stuff and stick with as-of
versions_end
.
Otherwise we are going to potentially fool people who are attempting to be careful by checking the as_of
of the epi_df
we give them, by giving them an as_of
that wasn't actually recorded and may be nowhere near to one that was recorded.
[Probably I could be convinced otherwise if that's what would actually be helpful in real production forecasting. Just marking this as "Comment" so someone else can go ahead & Approve if that's the case or just bypass & merge.]
Switching back to epix_as_of_current() and rebasing. |
* fix yearmonth breaking in epix_as_of() and epix_slide()
bb830ff
to
c6b04a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version needs to be bumped, but on a quick read-through looks good
Checklist
Please:
PR).
brookslogan, nmdefries.
DESCRIPTION
. Always incrementthe 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).
(backwards-incompatible changes to the documented interface) are noted.
Collect the changes under the next release number (e.g. if you are on
1.7.2, then write your changes under the 1.8 heading).
process.
Change explanations for reviewer
REVISED:
Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch
epix_slide
fails onyearmonth
s #648