-
Notifications
You must be signed in to change notification settings - Fork 2
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
Formatting Output For FluSight Submission From Arviz #10
Conversation
# default horizons | ||
if horizons is None: | ||
horizons = list(range(-1, 4)) | ||
reference_date_dt = datetime.strptime(reference_date, "%Y-%m-%d") |
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.
Better to expect reference_date
to be a datetime
rather than string?
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.
This is what I originally had (because it seemed cleaner to not have to convert to datetime
in the function) but opted against, since I have also the impression that most people would prefer passing the a string than a datetime
object, hope the function handles converted use adequately. My mind is open to this though. There are date usage patterns that others are more familiar with than I am.
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.
This is fine, but please document the required date format,
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 still don't see the date format requirement documented.
Co-authored-by: Damon Bayer <[email protected]>
Co-authored-by: Damon Bayer <[email protected]>
Thank you @damonbayer for these comments! |
Co-authored-by: Damon Bayer <[email protected]>
…gov/forecasttools-py into 7-data-loading-functions-and-storage
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.
Approving, but I have suggested a few places to clean up before merging. Thanks @AFg6K7h4fhy2!
Thank you Damon. All comments should be addressed via code changes or issues. |
This PR, while made on a branch originally for handling data functions, covers the full reinstantiation of
cfa-forecasttools
vignette for formatting FluSight submission. The code covered in this PR does not include adding "samples" versus "quantiles", nor does this PR's contents cover scoring (which is cover in #9 ). There are still some fixes needed before this code is as robust as we would like.#4, #7 , #8, and #1 can be closed when this PR is complete.
Questions
idata
per jurisdiction or support more complexposterior_predictive
inidata
(multiple jurisdictions)?Tasks
This PR.
vignette
.poetry
dependency grouping.Other PRs & Issues.
poetry build
& datasets (see Ensured Datasets Are Bundled Properly #14 )..qmd
vignette location (see Website With Rendered Vignettes #13 ).