search

daattali / ggExtra

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

Violin plot support #63

Closed alanocallaghan closed 7 years ago

alanocallaghan commented 7 years ago

Close #62

daattali commented 7 years ago

Thank you, I'll take a look and merge on the weekend. If you'd like, you can also add violin to the function documentation, examples, and a unit test

alanocallaghan commented 7 years ago

I'll do the above today. Cheers

alanocallaghan commented 7 years ago

Done, I also re-ran devtools::document(). I get one failing test and one warning seemingly due to a missing file

daattali commented 7 years ago

Hey, sorry for taking so long, I finally took a look. Thanks for adding the documentation!

One last thing is that the tests need to pass. I see you added a test for violin plot, but I don't see the associated expected image. For any visual test there's an expected image (see https://github.com/daattali/ggExtra/tree/master/tests/figs/ggMarginal) so that the test will be able to compare. Could you add the expected images for this? @crew102 can weigh in on how to generate them if you don't know

alanocallaghan commented 7 years ago

Thanks Dean.

I wasn't quite sure how the comparison of images worked (never seen tests laid out like this before) & thought maybe the tests would just pass on your machine. I've rendered them it out, but as noted in the commit message it also updated an unrelated file in the ggplot2-latest folder

By the way, why is the environment variable named RunGgplot2Tests=yes with a value of "yes", rather than RunGgplot2Tests? I don't think I can create a variable of that name on my Linux box

daattali commented 7 years ago

@Alanocallaghan thanks for the quick job! As for the env variable, that's a good question, and you're right I think it was an error.