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

bug(evalcast): remove title from labs in plots #641

Merged
merged 3 commits into from Jun 23, 2023
Merged

Conversation

dshemetov
Copy link
Collaborator

closes #640

  • removes title argument from the labs
  • also updates the DESCRIPTION file with remotes that work with devtools, remotes, and pak

@brookslogan
Copy link
Collaborator

Thanks; do you know what the plot labels look like now in the vignette?

@dshemetov
Copy link
Collaborator Author

They looked unchanged to me 🤷, but I didn't look suuuper closely

Copy link
Collaborator

@brookslogan brookslogan 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! This should fix vignette rendering and some user bugs. We might want to revisit in another Issue if the guide labels are unclear or eating up a bunch of space now [we want a title].

@brookslogan
Copy link
Collaborator

If you look at the wedge plots here, for example, the text "Proportion below" and "Proportion above" labeling the colors should have changed. It might be a better hack to just say "Proportion below" and "Proportion above" instead of the corresponding sprintf failures; I'm not sure what it's going to look like by default.

@brookslogan
Copy link
Collaborator

brookslogan commented Jun 23, 2023

Sorry, never mind, my bad. There shouldn't be a change; sprintf with NULL returns character(0) [and I assume title = character(0) does nothing as there is no default title]

@dshemetov
Copy link
Collaborator Author

dshemetov commented Jun 23, 2023

This is what the first plot looks like 🤷

image

@brookslogan
Copy link
Collaborator

brookslogan commented Jun 23, 2023

Yep, it makes sense now; I thought NULL acted like "" in sprintf but it actually acts like character(0) [this is more natural as length(NULL) is 0, but I thought it sprintf was less natural], so we're going from buggily saying "no title" ----> not saying anything about the title & getting no title.

@dshemetov
Copy link
Collaborator Author

Once CI finishes, I'll merge. I don't think we should continue adding features to this repo, so I wouldn't open a title issue.

@zterner-mitre
Copy link

Thanks for all your work on this!! I really appreciate it. I'd be happy to confirm that things work on my end once the changes are finished, just let me know how to proceed at that time if you can.

@dshemetov
Copy link
Collaborator Author

Thanks @zterner-mitre! Can you try running through the tutorial again after installing with

pak::pkg_install("cmu-delphi/covidcast/R-packages/evalcast@ds/evalcast-plot")

@zterner-mitre
Copy link

The first plot_calibration is giving this warning and this plot, which I think is probably not what you are aiming for?
image
image

The second plot_calibration seems good:
image

Third also seems good
image

@zterner-mitre
Copy link

plot_coverage and plot_width are producing too many plots given the setup of the plots (all in one row, for instance), and also may be missing some data. I can share those with you here, or open another issue, whichever you prefer

@dshemetov
Copy link
Collaborator Author

What does your predictions_cards look like? My first plot does not match yours.

@dshemetov
Copy link
Collaborator Author

I think the issue might be that your plots are using the full forecast_dates instead of the reduced forecast_dates_dec

@zterner-mitre
Copy link

Yep! That's it. I had manually defined forecast_dates_dec yesterday when I was having issues with get_covidhub_forecast_dates to be all dates within December. But using the sequence of 4 dates instead of 31 fixed everything. Thanks!

@dshemetov
Copy link
Collaborator Author

Excellent!

@dshemetov dshemetov merged commit 603f551 into main Jun 23, 2023
3 checks passed
@dshemetov dshemetov deleted the ds/evalcast-plot branch June 23, 2023 21:22
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.

plot_calibration gives .data errors, based on type of plot, when following evalcast tutorial
3 participants