Skip to content
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

Get flusight bin endpoints #15

Merged
merged 7 commits into from
Sep 26, 2024
Merged

Get flusight bin endpoints #15

merged 7 commits into from
Sep 26, 2024

Conversation

elray1
Copy link
Contributor

@elray1 elray1 commented Sep 24, 2024

This contains a general setup that I think should work for both seasons, but I ran out of time for testing the stuff for the 22/23 season, so I mostly took it out and dropped code into #16. There are a couple of vestigial leftover bits that I didn't want to delete since we will want to recreate it at some point...

Base automatically changed from transform_quantile_to_pmf to main September 25, 2024 14:19
Copy link
Collaborator

@lshandross lshandross left a comment

Choose a reason for hiding this comment

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

I'm approving because everything looks good to me, but I have one minor question: why did you decide to make the sign for the bin endpoints separate columns?

@elray1 elray1 marked this pull request as ready for review September 26, 2024 14:11
@elray1
Copy link
Contributor Author

elray1 commented Sep 26, 2024

Thanks!

why did you decide to make the sign for the bin endpoints separate columns?

I basically felt that this was clearer when working with the pmax operation here. If the sign is negative, what you would want is to add pmin(.data[["lower_rate_multiplier"]] * .data[["pop100k"]], .data[["lower_min_count_change"]]). But you'd also need to make sure that the sign was the same for both of those arguments. And... would it be the same pmin/pmax decision for both the lower and upper endpoints? The answer is yes, but I coded it up wrong in my first try.

@elray1 elray1 merged commit 77e9163 into main Sep 26, 2024
6 checks passed
@elray1 elray1 deleted the get_flusight_bin_endpoints branch September 26, 2024 14:15
@lshandross
Copy link
Collaborator

One other thing to keep in mind — FluSight had slightly different category definitions for the first few weeks of the 2023/2024 season, which were then adjusted at the beginning of November 2023. I don't know if it makes sense to have an option to use these in the function to get been endpoints for the 2023/2024 season, but it's also possible this could account for some of the mismatches between quantile forecasts and the categorical ones.

@elray1
Copy link
Contributor Author

elray1 commented Sep 26, 2024

I didn't remember that. do you know of a place where those different category definitions are recorded?

@lshandross
Copy link
Collaborator

Here's a link to the specific section in the model-output README before the categories were updated in the FluSight-forecast-hub repo

@elray1
Copy link
Contributor Author

elray1 commented Sep 26, 2024

Thanks, just noting that I filed issue #17 to address this point!

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.

2 participants