search

daattali / ggExtra

📊 Add marginal histograms to ggplot2, and more ggplot2 enhancements
http://daattali.com/shiny/ggExtra-ggMarginal-demo/
Other
383 stars 48 forks source link

Fix scale issues #101

Closed crew102 closed 6 years ago

crew102 commented 6 years ago

This PR should close issues #9, #80, #81, #92, and #94. There were a few reasons why our previous approach to copying the scale transformations wasn't working, though even now I don't fully understand what was happening. Also somewhat mysteriously, the code in this PR results in the scale transforms being copied from the scatter plot over correctly to the marginal plots, even though there is no addition of a custom scale to the marginal plots (i.e., getScale() is totally deprecated in this PR). I think the environment in which the scale/plot is evaluated is somehow tied up in it all, but again I'm not sure what exactly is going on.

daattali commented 6 years ago

A few of the test figures seem to be failing, and the CRAN check is also showing me a new check that I didn't know existed: file paths must be <100 characters long https://travis-ci.org/daattali/ggExtra/builds/353103973#L813

For the path length issue, an easy (ugly hacky, but easy) solution would be to use camelCase in the file names

crew102 commented 6 years ago

Ugh, I forgot to update runMarginalTests() again - that's why it's failing. I'll update and shorten file names.

daattali commented 6 years ago

Thanks @crew102 ! Out of curiousity, any special reason for "move aes calls out of geom funs"?

crew102 commented 6 years ago

Moving the aes() call in basicScatP() out of geom_point() and into ggplot() in allowed me to not have to call aes() in geom_smooth() in this test:

https://github.com/daattali/ggExtra/blob/c9e168fef379f7cc6fbf76b86db749091fa6a2a0/tests/testthat/helper-funs.R#L78-L81

So basically it saved a few chars of code (I'm a bit of a DRY natzi).

daattali commented 6 years ago

I'm 100% supporter of extreme dryism

On Mar 29, 2018 10:57, "Chris Baker" notifications@github.com wrote:

Moving the aes() call in basicScatP() out of geom_point() and into ggplot() in allowed me to not have to call aes() in geom_smooth() in this test:

https://github.com/daattali/ggExtra/blob/c9e168fef379f7cc6fbf76b86db749 091fa6a2a0/tests/testthat/helper-funs.R#L78-L81

So basically it saved a few chars of code (I'm a bit of a DRY natzi).

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/daattali/ggExtra/pull/101#issuecomment-377262692, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6IFCIbhi70QJFUUrQhJtTu0P7fKwACks5tjPZLgaJpZM4Spmg- .