palaeoverse / rphylopic

Get Silhouettes of Organisms from PhyloPic
https://rphylopic.palaeoverse.org
GNU General Public License v3.0
91 stars 9 forks source link

distorted aspect ratio in figures with very different x/y axes acaling #29

Closed KamilSJaron closed 4 years ago

KamilSJaron commented 4 years ago

Hi there, thanks for rphylopic! I have problem between versions 0.2.0 and 0.3.0. It seems to me that the versions are not backwards compatible. Version 0.2 was accepting relative x/y coordinates, while the 0.3 takes absolute (at least for the add_phylopic_base function). In the version 0.3.0 the scaling is distorted. It seems to me that it holds the ratio of axis, which is really problematic for plots with very different x/y.

In the v0.2.0 everything works fine regardless of axis scaling

library('rphylopic')
Ocin_silh <- image_data("db8f0cd7-3780-4ddc-97a5-1f358c40c734", size = 128)[[1]]
Afus_silh <- image_data("af19a15c-9cbd-485d-ad7f-4f2691a19d95", size = 128)[[1]]

pdf('issue_28_ok_figure.pdf', width = 10, height = 6)
  par(mfrow = c(1, 2), oma = c(0, 0, 2, 0))
  plot(as.numeric(NULL), xlim = c(0, 1), ylim = c(0, 500))
  add_phylopic_base(Ocin_silh, x = 0.2, y = 0.9, ysize = 0.3, alpha = 1)

  plot(as.numeric(NULL), xlim = c(0, 300), ylim = c(0, 700))
  add_phylopic_base(Afus_silh, x = 0.15, y = 0.93, ysize = 0.23, alpha = 1)
dev.off()
Screenshot 2020-07-15 at 13 04 30

The v0.3.0 seems to have a problem when x/y axes are very different (panel a). The problem is not apparent for cases when the axes have a similar scale (panel b).

library('rphylopic')
Ocin_silh <- image_data("db8f0cd7-3780-4ddc-97a5-1f358c40c734", size = 128)[[1]]
Afus_silh <- image_data("af19a15c-9cbd-485d-ad7f-4f2691a19d95", size = 128)[[1]]

pdf('issue_28_not_so_great_figure.pdf', width = 10, height = 6)
  par(mfrow = c(1, 2), oma = c(0, 0, 2, 0))
  plot(as.numeric(NULL), xlim = c(0, 1), ylim = c(0, 700))
  add_phylopic_base(Ocin_silh, x = 0.2, y = 0.9 * 700, ysize = 0.3 * 700, alpha = 1)

  plot(as.numeric(NULL), xlim = c(0, 300), ylim = c(0, 700))
  add_phylopic_base(Afus_silh, x = 0.15 * 300, y = 0.93 * 700, ysize = 0.23 * 700, alpha = 1)
dev.off()
Screenshot 2020-07-15 at 13 04 39
Session Info of the alright plot ```r sessionInfo() R version 3.5.2 (2018-12-20) Platform: x86_64-apple-darwin15.6.0 (64-bit) Running under: macOS 10.15.4 Matrix products: default BLAS: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRblas.0.dylib LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib locale: [1] C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] rphylopic_0.2.0 loaded via a namespace (and not attached): [1] Rcpp_1.0.1 png_0.1-7 assertthat_0.2.1 dplyr_0.8.1 [5] crayon_1.3.4 gridBase_0.4-7 grid_3.5.2 crul_0.9.0 [9] R6_2.4.0 jsonlite_1.6 gtable_0.3.0 magrittr_1.5 [13] scales_1.0.0 ggplot2_3.2.0 pillar_1.4.1 rlang_0.4.5 [17] curl_3.3 lazyeval_0.2.2 glue_1.3.1 purrr_0.3.2 [21] munsell_0.5.0 compiler_3.5.2 pkgconfig_2.0.2 colorspace_1.4-1 [25] tidyselect_0.2.5 tibble_2.1.3 httpcode_0.3.0 ```
Session Info of the distorted plot ```r R version 4.0.2 (2020-06-22) Platform: x86_64-apple-darwin19.5.0 (64-bit) Running under: macOS Catalina 10.15.4 Matrix products: default BLAS/LAPACK: /usr/local/Cellar/openblas/0.3.10_1/lib/libopenblasp-r0.3.10.dylib locale: [1] C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] rphylopic_0.3.0 loaded via a namespace (and not attached): [1] png_0.1-7 crayon_1.3.4 gridBase_0.4-7 grid_4.0.2 [5] crul_0.9.0 R6_2.4.1 jsonlite_1.7.0 lifecycle_0.2.0 [9] gtable_0.3.0 magrittr_1.5 scales_1.1.1 pillar_1.4.6 [13] ggplot2_3.3.2 rlang_0.4.7 curl_4.3 vctrs_0.3.1 [17] ellipsis_0.3.1 glue_1.4.1 munsell_0.5.0 compiler_4.0.2 [21] pkgconfig_2.0.3 colorspace_1.4-1 tibble_3.0.3 httpcode_0.3.0 ```
sckott commented 4 years ago

thanks for the report @KamilSJaron and for including session info!

@dill any ideas? This seems clearly linked to the change made in #26 #28 From this report, seems like perhaps there was a good use case for the old version of add_phylopic_base() that used par() - I wonder if we can sort out the high level use cases here and ideally support all of reasonable ones. Is there just two use cases that we can use a parameter to toggle between?

dill commented 4 years ago

ah bummer, I thought we'd got this sorted. I'll take a look and post back when I have a chance. Sorry about the hassle @KamilSJaron!

sckott commented 4 years ago

thanks @dill

KamilSJaron commented 4 years ago

No worries, thanks to you for making am r interface for phylopic!

dill commented 4 years ago

Sorry for the delay, a bunch of other work stuff came up. I've tried to fix this in this PR.

I've also added an xsize argument to allow for any fine-grained adjustment in case the automatic version doesn't work (again!).

Let me know if this works for you :)

sckott commented 4 years ago

@KamilSJaron if you have time can you test the solution in pull request #30 - should be able to install like remotes::install_github("dill/rphylopic")

KamilSJaron commented 4 years ago

Hi folks, sorry for the delay, I was doing a fieldwork (collecting the right silhouettes). I will take a look shortly.

KamilSJaron commented 4 years ago

Hi there, I tried and it does work. With xsize it will be always possible to plot a silhouette in any possible size including all distortions.

To be completely honest, I liked the previous behaviour better than this one (specifying always relative coordinates). I will think if there would be an easy way to allow both absolute and relative coordinates / sizes.

dill commented 4 years ago

Thanks for checking! I liked the previous specification, but since it was causing issues for people in terms of output we had to switch (rather have functionality in all cases over ease of use) but if you think of a way of getting it working (even just as a quick example, not a full PR) feel free to open an issue and I'll look into it :)

@sckott can the PR be merged now?

sckott commented 4 years ago

yes

dill commented 4 years ago

🍻