Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
291 date period #297
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
291 date period #297
Changes from all commits
dff767e
0667845
edd5134
e3b2907
a624b63
69b21c6
3498fcf
b2b8134
f2f39a2
f9859fe
fb7faae
78ff1f9
3d059f4
de9e00a
0c38723
e036b8d
e6e60cd
845c1a9
8feef06
6d5c379
047caa6
cfde0c9
fc05cd9
1e76059
f5ef88b
c1e3ff9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does having this go from
max_time_value + ahead
toforecast_date + ahead
cause this to function to behave differently? If yes and testing this PR becomes tough, then we could reduce scope here and punt these changes to another PR.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.
Separate question: how do the units in ahead change depending on the time type? Is this primarily up to the user to make sure they specify aheads in the right units?
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.
On
max_time_value + ahead
, this was the default previously, and remains the fall back (see lines below). However, if there is aforecast_date
, then I think the default fortarget_date
should be to use the specifiedforecast_date
first.This PR is meant to allow for
ahead
to always be an integer. Then it should be in units relative to thetime_type
of theepi_df
. It took a lot of effort to make that happen though.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.
I think this is wrong, but I'm not yet certain that it's safe to remove.
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.
doing some timetravel, it looks like it's been here in one form or another forever. What's confusing to me is that this looks like it does nothing. Was the point to make all the characters columns lowercase (such as
geo_value
) to match correctly maybe, but there was a parenthesis problem? Wasdplyr::mutate(dplyr::across(tidyselect::where(is.character)), tolower)
meant?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.
Yeah, I'm not sure why it was there in the first place. I suspect that the included state population dataset used capitals while typical usage from the API gives geos in lower case. But that shouldn't have resulted in hardcoded workarounds here (that are prone to failure).
So I think this should go forever, but I wanted to be sure that if some downstream use errored out, I could find this and try to track it more carefully.