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

Fixes #9

Merged
merged 5 commits into from
Nov 18, 2018
Merged

Fixes #9

merged 5 commits into from
Nov 18, 2018

Conversation

patrickfletcher
Copy link
Contributor

Hi Bastian!

I really like your object oriented violin plots! I am inspired to rewrite several of my own plotting functions following this pattern. I found a few little things that I thought could be fixed (see commit messages).

A few more thoughts for improvements, which I plan to complete in my fork if you're interested:

  • make a separate "MiniBoxPlot" class to simplify the Violin code. Violin will then contain one of these. This will be used via a plot function (like violinplot.m) as a simple standalone object-oriented alternative to Matlab's boxplot function (in plotstyle='compact' mode).
  • in violinplot, support plotting a matrix of violins, when rows (observations) are grouped according to a grouping variable, and columns represent variables (with names optionally given).

Best,

Patrick

ShowData was checking Notch visibility, and ShowNotches was checking ScatterPlot visibility.  Also, the "logical" cast is unnecessary
for now in units of x axis; could also be done as fraction of violin width.
If there's only one datapoint, all plot objects except median marker are not generated so their associated get/set methods give errors
@bastibe
Copy link
Owner

bastibe commented Nov 18, 2018

Cool! I like it!

Just one question: In bd5059a, did you change the default box width from 0.01 to 0.03? Could you post a screen shot of the two versions?

As for factoring out a MiniBoxPlot, I like the idea in general, but in my personal experience, more generality is not always to the benefit of the code. We would have to decide with an actual code example if the generalization is worth the tradeoff in readability.

Regarding a matrix of violin option, I am certainly open to that, so long as it doesn't break or change existing functionality.

also move the default Violin width (0.3) into the input parser (removes need to check if args.Width is empty)
@patrickfletcher
Copy link
Contributor Author

RE: box width. I updated things to be exactly as in your original version (new vs original images attached). Also I thought the default violin width (0.3) could be encoded in the input parser (see new commit).

new
original

As for refactoring, I'm not sure yet whether it'll help or hinder. For that idea and also for the matrix violin options, I'll propose those as a separate issue (for discussion) or pull requests (for the matrix) when I get more done.

@bastibe bastibe merged commit ece71df into bastibe:master Nov 18, 2018
@bastibe
Copy link
Owner

bastibe commented Nov 18, 2018

Thank you so very much! I love it!

@patrickfletcher patrickfletcher deleted the fixes branch November 18, 2018 20:05
@patrickfletcher
Copy link
Contributor Author

Glad to help. Thank you for this project :)

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

Successfully merging this pull request may close these issues.

2 participants