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

Resolves #1466 #1534

Merged
merged 13 commits into from Sep 24, 2020
Merged

Resolves #1466 #1534

merged 13 commits into from Sep 24, 2020

Conversation

ydhuang28
Copy link
Contributor

Description

Incident cases forecasts can now be seen on the visualization.

@ydhuang28 ydhuang28 added the viz label Sep 23, 2020
@ydhuang28 ydhuang28 linked an issue Sep 23, 2020 that may be closed by this pull request
@ydhuang28 ydhuang28 changed the title Add incident cases forecasts to visualization, resolves #1466 Resolves #1466 Sep 23, 2020
Copy link
Contributor

@hannanabdul55 hannanabdul55 left a comment

Choose a reason for hiding this comment

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

Looks good! One minor suggestion, I think with this change, the default target chosen when you load the viz is inc cases, this is because the default target is set to the last one (size_of_seasons-1) when loaded, could you change that to the Cumulatve deaths target? It's in Chloropeth.vue#ready() function. Otherwise this looks good to me and let's get this merged! 👍

@@ -68,6 +68,12 @@ exports.targetFullNameCum = {
'3-ahead': '3 wk ahead cum death',
'4-ahead': '4 wk ahead cum death'
};
exports.targetFullNameIncCase = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this variable used anywhere?

Copy link
Contributor Author

@ydhuang28 ydhuang28 Sep 23, 2020

Choose a reason for hiding this comment

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

Yep, it's used here to pick out the incident case data from the filtered CSV files. For some reason GitHub doesn't expand the diff automatically...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe because it has csv in the filename?

Add weekly report for 2020922 and update rmd
@nickreich
Copy link
Member

Is @hannanabdul55 's comment above about which of the forecasted targets are loaded when the page first loads? if so, I might prefer to have this load the incident deaths by default instead of cumulative deaths. minor point, and may warrant an issue of it's own?

@hannanabdul55
Copy link
Contributor

@nickreich I think this is straightforward to do here itself. Then it is even easier, just load the season in the above mentioned function to 0.

@ydhuang28
Copy link
Contributor Author

Hmm. I had the same idea and tried that, but apparently the data won't load on start-up if I update the selected target to 0. Both 1 (Cumulative Deaths) and 2 (Incident Cases) load the data correctly but 0 doesn't. However, if I start with Cumulative Deaths or Incident Cases and switch to Incident Deaths it's totally fine. Interestingly, if I don't call updateSelectedSeason(), this.selectedSeasonId defaults to 0; I wonder this is why the updateSelectedSeason() was added? @hannanabdul55 any thoughts?

@ydhuang28
Copy link
Contributor Author

I'll keep working on this in the mean time.

@hannanabdul55
Copy link
Contributor

@ydhuang28 yeah, this seems like a weird behavior. I think it is because of the season download data that happens at load just download only the last one? I am not sure exactly what's going on there. Will need to dig into that more. But I think that is not a big issue right now and could be tracked in another Github issue itself and in the meantime, we could maybe merge this? I am fine with either way.

@ydhuang28
Copy link
Contributor Author

Yep, we can merge this. I'll open a separate issue for the data loading problem.

@ydhuang28 ydhuang28 merged commit ac2ac6c into master Sep 24, 2020
@ydhuang28 ydhuang28 deleted the feature_add-inc-case-forecasts branch September 24, 2020 00:44
@ydhuang28
Copy link
Contributor Author

I've created a new issue for showing the correct default target: Issue #1536

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add state/US-level incident case forecasts to viz
4 participants