-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Do not change order of positive and negative values in PositionStack #3471
Conversation
@@ -195,7 +195,7 @@ PositionStack <- ggproto("PositionStack", Position, | |||
) | |||
} | |||
|
|||
rbind(neg, pos) | |||
rbind(neg, pos)[match(seq_len(nrow(data)), c(which(negative), which(!negative))),] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see immediately that this is always correct. Can there be problems with NA
s? Or will NA
s never reach this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An NA
value in ymax is pretty nonsensical and would also have broken the previous code. I can put in some guarding if you’d like nonetheless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion either way. Just make sure to look into why the CI builds fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failures are due to the downstream changes in sf, which have been fixed in master but not yet merged into the v3.2.1-rc
branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'll close #3401
This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/ |
fixes #3390
@karawoo I have checked that this fix does not cause regressions in #1552. Basically what this does is avoid positive and negative y-values to get reordered, not fiddle with the group-based reordering