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

Bad "inward"/"outward" justification with angle > 45 or angle < -45 #194

Open
aphalo opened this issue May 4, 2021 · 5 comments
Open

Bad "inward"/"outward" justification with angle > 45 or angle < -45 #194

aphalo opened this issue May 4, 2021 · 5 comments
Labels

Comments

@aphalo
Copy link
Contributor

aphalo commented May 4, 2021

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:

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(hjust = "inward")

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 = "outward", angle = 70)

p + geom_text_repel(hjust = "inward", angle = 70)

p + geom_text_repel(hjust = "outward", angle = 90)

p + geom_text_repel(hjust = "inward", angle = 90)

p + geom_text_repel(hjust = "outward", angle = -90)

p + geom_text_repel(hjust = "inward", angle = -90)

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:

R version 4.0.5 (2021-03-31)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19042)

Matrix products: default

locale:
[1] LC_COLLATE=English_Finland.1252  LC_CTYPE=English_Finland.1252   
[3] LC_MONETARY=English_Finland.1252 LC_NUMERIC=C                    
[5] LC_TIME=English_Finland.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] shiny_1.6.0        ggrepel_0.9.1.9999 ggplot2_3.3.3.9000

loaded via a namespace (and not attached):
 [1] styler_1.4.1      tidyselect_1.1.0  xfun_0.22         bslib_0.2.4      
 [5] purrr_0.3.4       colorspace_2.0-0  vctrs_0.3.8       generics_0.1.0   
 [9] miniUI_0.1.1.1    htmltools_0.5.1.1 yaml_2.2.1        utf8_1.2.1       
[13] rlang_0.4.10      later_1.2.0       pillar_1.6.0      jquerylib_0.1.4  
[17] glue_1.4.2        withr_2.4.2       DBI_1.1.1         lifecycle_1.0.0  
[21] munsell_0.5.0     gtable_0.3.0      evaluate_0.14     labeling_0.4.2   
[25] knitr_1.33        callr_3.7.0       fastmap_1.1.0     ps_1.6.0         
[29] httpuv_1.6.0      parallel_4.0.5    fansi_0.4.2       highr_0.9        
[33] Rcpp_1.0.6        xtable_1.8-4      clipr_0.7.1       scales_1.1.1     
[37] promises_1.2.0.1  backports_1.2.1   cachem_1.0.4      jsonlite_1.7.2   
[41] mime_0.10         farver_2.1.0      fs_1.5.0          digest_0.6.27    
[45] processx_3.5.1    dplyr_1.0.5       grid_4.0.5        cli_2.5.0        
[49] tools_4.0.5       magrittr_2.0.1    sass_0.3.1        tibble_3.1.1     
[53] crayon_1.4.1      pkgconfig_2.0.3   ellipsis_0.3.2    reprex_2.0.0     
[57] rmarkdown_2.7     assertthat_0.2.1  rstudioapi_0.13   R6_2.5.0         
[61] compiler_4.0.5  
@aphalo
Copy link
Contributor Author

aphalo commented May 4, 2021

There seems to be an additional problem when both hjust and vjust are passed to geom_text_repel() adding nudging even when nudge is set to zero. In these examples "right" and "left" work as expected. This is likely to be also behind the odd behavior above when angles are different from 0 or 90 degrees.

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)

@slowkow
Copy link
Owner

slowkow commented May 4, 2021

@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 .Rmd file for the purpose of testing nudging and justification? Maybe one file for geom_text, and another file for geom_label? I don't know how to minimize the amount of work for the developer here, because a human probably needs to use their eyes to view an image and judge whether a test is passing or failing. Any solutions you can come up with would be great!


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 unit() function altogether, and instead use arbitrary units. This might simplify all calculations throughout the package, but I'm not certain if it will work. I thought you might want to know this, in case you start noticing that the ggrepel calculations are unnecessarily complicated. The latest ggplot2 code is probably a good place to get inspired on how best to do these calculations.

@slowkow slowkow added the bug label May 4, 2021
@aphalo
Copy link
Contributor Author

aphalo commented May 4, 2021

@slowkow It took me quite a lot of trial an error to get the justification working in ggplot2. I started poking in geom_text() when I started seeing odd behavior from my new nudge functions. There are to sides to it, the position of the text label itself and the midpoint from which of x or y to use as reference.

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 for loop, but I have not tested the difference in performance. It seems possible, for example, to draw all segments with a single function call.

@aphalo
Copy link
Contributor Author

aphalo commented May 8, 2021

@slowkow #196 updates compute_just() and deals only with the handling of specific values of angle together with "inward" and "outward" justification. So, even though this change can affect the rendering of specific plots, from what I can see it does not affect any of the 'ggrepel' examples. By the way, the last example in tidyverse/ggplot2#4447 is from a previous issue. It is indeed illuminating and was very useful when developing the new version of compute_just().

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 compute_just().

@aphalo
Copy link
Contributor Author

aphalo commented May 8, 2021

@slowkow #196 seems to correct the "inward"/"outward" swap problem when angle != 0. One can also see that when angle takes certain values the segment is not clipped to the boundary of the text box and not linked to the center of the text box. This may be related to some other open issues.

The swap between "inward"/"outward" likely explains #191.
Issue #171 might be also related to what we see below with rotated text. Somehow justification affects in unexpected ways the clipping and location of segments.

So here is the first reprex above (except that I left out some geom_text() examples) run with #196 applied.

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)

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

No branches or pull requests

2 participants