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

Fix evalcast:::get_forecast_dates bug #587

Closed
wants to merge 1 commit into from

Conversation

dshemetov
Copy link
Collaborator

@dshemetov dshemetov commented Jul 19, 2022

An alternative approach to the same ends as in #586.

Probably more long term safe, but maybe moving too fast in deprecating get_covidhub_forecast_dates. A more complete version of this work would switch us completely to using zoltr, while putting in the html parse fix, to continue to support code that continues to use get_covidhub_forecast_dates. I just don't have the bandwidth to take that on right now, especially since the zoltr get predictions function appears to be broken now.

r$> get_zoltar_predictions("CMU-TimeSeries", forecast_dates = "2022-07-18")
get_token(): POST: https://zoltardata.com/api-token-auth/
get_resource(): GET: https://zoltardata.com/api/projects/
get_resource(): GET: https://zoltardata.com/api/project/44/timezeros/
[1] "Grabbing forecasts from Zoltar..."
Error: POST status was not 200. status_code=400, json_response=Invalid query. error_messages='["target with name not found. name=1 wk ahead inc hosp, valid names=['17 day ahead inc hosp', '17 wk ahead cum death', '17 wk ahead inc death', '18 day ahead cum death', '18 day ahead inc death', '2 wk ahead inc case', '101 day ahead inc hosp', '102 day ahead cum death', '102 day ahead inc death', '102 day ahead inc hosp', '103 day ahead cum death', '16 wk ahead cum death', '16 wk ahead inc death', '17 day ahead cum death', '17 day ahead inc death', '18 day ahead inc hosp', '18 wk ahead cum death', '1 wk ahead inc case', '3 wk ahead inc case', '4 wk ahead inc case', '5 wk ahead inc case', '6 wk ahead inc case', '7 wk ahead inc case', '8 wk ahead inc case', '18 wk ahead inc death', '19 day ahead cum death', '19 day ahead inc death', '19 day ahead inc hosp', '19 wk ahead cum death', '19 wk ahead inc death', '20 day ahead cum death', '20 day ahead inc death', '20 day ahead inc hosp', '20 wk ahead

cc @nmdefries @brookslogan for input.

* redirect get_forecast_dates in covidhub_predictions to zoltr.
@dshemetov dshemetov marked this pull request as draft July 19, 2022 23:39
@nmdefries
Copy link
Contributor

For now, I think it's fine to combine the changes here with the html fix in case anyone is still using get_covidhub_forecast_dates.

The target with name not found error is because of improper target construction. Hospitalizations are forecast on a daily basis (1 day ahead inc hosp) but when epiweek incidence_period is selected we construct invalid weekly hospitalization targets. Same error happens for cases and deaths if incidence_period is set to day. I'm opening an issue with more info.

@brookslogan
Copy link
Collaborator

brookslogan commented Jul 20, 2022

Do we know whether covidhub and zoltar always have the same results? If not, it's probably better to just do the xpath fix, and keep this zoltar mix-and-match as an emergency backup plan.

@brookslogan
Copy link
Collaborator

I couldn't immediately find anything guaranteeing compatibility between covidhub and zoltar, and I assume that there are at the very least delays in syncing zoltar. So I think for now we should just do #588 and refer back to this PR (#587) if GItHub updates the HTML again and breaks things.

@dshemetov dshemetov changed the title Fix get_forecast_dates bug Fix evalcast:::get_forecast_dates bug Jul 20, 2022
@nmdefries
Copy link
Contributor

nmdefries commented Jul 20, 2022

The GitHub REST API also provides a stable way of grabbing file names from a repo, e.g. this chunk uses the API to get names of changed files. It would be more work to implement and needs authentication.

@dshemetov
Copy link
Collaborator Author

dshemetov commented Aug 1, 2022

Thanks for the link @nmdefries!

Did some digging in that so I understand it better. Here are my notes.

You can get the files/folders present in a repo with the trees endpoint:

# Example:
https://api.github.com/repos/reichlab/covid19-forecast-hub/git/trees/master

So to find the dates, you'd need to make the request above, find the hash for data-processed, make another request, find the hash for the particular forecaster, request again, and from there collect the available forecasts, parsing out the dates from the csv files.

@brookslogan
Copy link
Collaborator

I believe we ended up fixing this instead with #588, which merged the changes from #586 from the evalcast-forecast-eval (used by forecast eval dashboard) branch into evalcast (used by production forecast display Rmd). I think we can just close this PR.

@nmdefries nmdefries closed this Aug 11, 2022
@dshemetov dshemetov deleted the ds/fix-get-covidhub-dates branch August 16, 2022 23:09
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.

None yet

3 participants