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.

Sign up for GitHub

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

Jump to bottom

Fix broken quantiles (#88) #89

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Fix broken quantiles (#88) #89

merged 4 commits into from
Apr 1, 2020

Conversation

jstockwin
Copy link
Collaborator

Closes #88

@jstockwin jstockwin requested a review from zkamvar March 17, 2020 12:53
@jstockwin
Copy link
Collaborator Author

@zkamvar Would you mind taking a look at this when you get the chance?

@jstockwin jstockwin mentioned this pull request Mar 17, 2020
Closed
zkamvar
zkamvar requested changes Mar 23, 2020
View reviewed changes
Copy link
Collaborator

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

This looks good to me, except now the tests need to be adjusted to reflect the true quantiles. Would you mind fixing the binary files in test/expected_output?

What you cannot fix: the plots are failing, but that's due to a bug in the new version of ggplot2 (tidyverse/ggplot2#3873) that caused incidence plots to fail (reconhub/incidence#119).

The current version of incidence on GitHub gets around this by centering the dates on the bars, but has a shifting effect.

@zkamvar
Copy link
Collaborator

zkamvar commented Mar 24, 2020

This looks good to me. There really is nothing we can do about the incidence problem until ggplot2 gets fixed.

@jstockwin
Copy link
Collaborator Author

jstockwin commented Mar 24, 2020 •
edited

Thanks. @zkamvar. Do you think we should wait for the issue to be resolved, or just merge with failing tests?

Currently I think I can't merge due to requested changes ("Merging can be performed automatically once the requested changes are addressed."), and probably it will say the same for failing tests... Edit: Github was being weird, it now says approved, but I can't merge due to failing tests.

@zkamvar
Copy link
Collaborator

zkamvar commented Mar 27, 2020

Thanks. @zkamvar. Do you think we should wait for the issue to be resolved, or just merge with failing tests?

Currently I think I can't merge due to requested changes ("Merging can be performed automatically once the requested changes are addressed."), and probably it will say the same for failing tests... Edit: Github was being weird, it now says approved, but I can't merge due to failing tests.

I think now that there is a new version of incidence on CRAN, the issue should be resolved. I believe the solution is to open R in the project and use vdiffr::manage_cases() and just make sure that they look generally okay (again, the bars will be shifted to the left a bit because of the nasty hack we had to do to get things to work properly [although I supposed the original solution itself was a nasty hack, so we are negating the nasty]).

@jstockwin
Copy link
Collaborator Author

@zkamvar I've updated the figures using vdiffr::manage_cases(). The bars appear in the same place, but the x-axis markers have moved to the right (which I guess is the same as the bars moving to the left and the whole thing then being moved right).

Hopefully the tests pass now...

@jstockwin
Copy link
Collaborator Author

@zkamvar Tests passed (here), but on my screen at least github is still saying "Waiting for status to be reported". Not sure if it's a Travis problem or a GitHub problem... hopefully it sorts itself out :/

@jstockwin
Copy link
Collaborator Author

@zkamvar I have restarted the travis build, since something clearly went wrong. I can now see that GitHub knows it's running, so this time everything should actually go green. Not sure what happened there...

Are you happy to merge once green? Do you know how to get an updated version on CRAN?

@jstockwin
Copy link
Collaborator Author

jstockwin commented Mar 31, 2020 •
edited

Also I've just pushed a new commit because I forgot to update NEWS.md...

@jstockwin jstockwin mentioned this pull request Mar 31, 2020
Merged
3 tasks
@zkamvar
Copy link
Collaborator

zkamvar commented Apr 1, 2020

LGTM! Thank you ^_^

@zkamvar zkamvar merged commit ce2ab43 into master Apr 1, 2020
@zkamvar zkamvar deleted the fix-broken-quantiles branch April 1, 2020 00:47
This was referenced May 14, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkamvar zkamvar zkamvar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Quantile.0.25(R) always equals Quantile.0.75(R)
2 participants
@jstockwin @zkamvar