-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
Bad "inward"/"outward" justification with angle > 45 or angle < -45 #194
Comments
There seems to be an additional problem when both This is run with current development code. Plots look the same with version 0.9.1 from CRAN. library(ggplot2)
library(ggrepel)
my.cars <- mtcars[c(TRUE, FALSE, FALSE, FALSE), ]
p <- ggplot(my.cars, aes(wt, mpg, label = rownames(my.cars))) +
geom_point(colour = "red")
p + geom_text(hjust = "right") p + geom_text_repel(nudge_x = 0, hjust = "right", max.iter = 0) p + geom_text_repel(nudge_x = 0, hjust = "left", max.iter = 0) p + geom_text_repel(nudge_y = 0, vjust = "bottom", max.iter = 0) p + geom_text_repel(nudge_y = 0, vjust = "top", max.iter = 0) p + geom_text_repel(nudge_y = 0, vjust = "bottom", nudge_x = 0, hjust = "right", max.iter = 0) p + geom_text_repel(nudge_y = 0, vjust = "top", nudge_x = 0, hjust = "left", max.iter = 0) Created on 2021-05-04 by the reprex package (v2.0.0) |
@aphalo Thank you for the detailed issue!!! It's a pleasure to see an issue with multiple code examples that help to illustrate the problem. It is very illuminating to look at your examples in the ggplot2 pull request: tidyverse/ggplot2#4447 (I especially like your last example with polar coordinates, which helped me to appreciate what the terms "inward" and "outward" are supposed to mean for text justification.) As you've demonstrated clearly, the ggrepel code for justification and nudging is certainly problematic (and probably unnecessarily complicated). It's nice to hear that ggplot2 developers might also struggle to get text justification right. 😛 Do you think it makes sense to create a new By the way... this issue reminds me of an email with Paul Murrell (author of the grid package) a few years ago, who suggested that it might be possible to rewrite the vast majority of ggrepel code to stop calling the |
@slowkow It took me quite a lot of trial an error to get the justification working in ggplot2. I started poking in In fact I am implementing also something more ambitious to go with the new nudge functions: allowing users to set an arbitrary focal point out from which to justify inwards or outwards. This is work in progress and I will let you know when it is working as it should. As for testing I have been using testhat together with vdiffr. TCurrently these packages do a good job at simplifying testing as visual checking is needed only when there is a mismatch. They are rather slow, and with 'ggrepel', I think they would be prohibitively slow unless setting iter=0. But tests for nudging and justification could be anyway best run with iter=0, so this I think is a good alternative. These tests would most likely have to not be run in CRAN, but with ggpmisc and ggspectra I have found them a godsend. It would have been otherwise impossible for me to maintain packages that define so many geometries, statistics, scales and now position functions. I will prepare some test cases for checking the code in the pull requests, to start with. To test nudging and justification. I most likely can recycle some cases from my packages or that I also need to add to my packages. I have decided to split 'ggpmisc' into two packages. One with extensions to the grammar of graphics and one with the various "decorations" based on model fits or other statistics analyses. The first of the packages is ready, but I am stuck with finding a good name for it. I plan to keep 'ggpmisc' alive either as the second package, or as a loader of the two packages, so as not to break user code. Thanks for the hint on calculations using 'grid'. I noticed that some functions in 'grid' are vectorized so one can probably avoid one |
@slowkow #196 updates Next time I find some free time I will make a pull request with test cases using 'vdiffr' and some updated examples using the nudge functions. I quickly looked at the code in 'ggrepel' but I could find any obvious place where the interpretation of justification would get reversed. Surelyly it was/is not in |
@slowkow #196 seems to correct the "inward"/"outward" swap problem when The swap between "inward"/"outward" likely explains #191. So here is the first reprex above (except that I left out some library(ggplot2)
library(ggrepel)
my.cars <- mtcars[c(TRUE, FALSE, FALSE, FALSE), ]
p <- ggplot(my.cars, aes(wt, mpg, label = rownames(my.cars))) +
geom_point(colour = "red")
# problem 1 opposite direction for geom_text() and geom_text_repel()
p + geom_text(hjust = "outward") p + geom_text_repel(hjust = "outward") p + geom_text_repel(hjust = "inward") # Angles > 45 degrees or < -45 degrees
# Same problem as the one now fixed in 'ggplt2'
p + geom_text_repel(hjust = "outward", angle = 30) p + geom_text_repel(hjust = "inward", angle = 30) p + geom_text_repel(hjust = "inward", angle = 30) p + geom_text_repel(hjust = "outward", angle = 70) p + geom_text_repel(hjust = "inward", angle = 70) p + geom_text_repel(hjust = "outward", angle = 90) p + geom_text_repel(hjust = "outward", angle = -90) p + geom_text_repel(hjust = "inward", angle = -90) Created on 2021-05-08 by the reprex package (v2.0.0) |
Summary
This is a bug originally in 'ggplot2', and fixed by a pull request submitted today. There is an additional problem of inconsistency in the direction for
"inward"
and"outward"
and"right"
and"left"
between 'ggrepel' and 'ggplot2'.Minimal code example
Here are some examples of code needed to demonstrate the issue:
Created on 2021-05-04 by the reprex package (v2.0.0)
Suggestions
As soon as the pull request is accepted in 'ggplot2' I will submit the matching pull request for 'ggrepel'.
I suggest at the same time making the response to
"inward"
and"outward"
and"left"
and"right"
the same as in 'ggplot2' instead of opposite. See examples above.Version information
Here is the output from
sessionInfo()
in my R session:The text was updated successfully, but these errors were encountered: