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

Adds documentation and violin plots! #107

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jpoles1
Copy link

@jpoles1 jpoles1 commented Apr 15, 2018

I think the documentation is the more significant contribution here. Fixes #104.

That said, I do like violin plots and use them often in my research. They are added here using figure_factory. I hope that this code is up to snuff and is reasonably in line with the style/organization of the rest of the codebase. Happy to tune this up further however you'd like @santosjorge. Fixes #50.

Normally I'd try and keep these two contributions separate, but I wanted to use the documentation in order to better develop the violin plots, so I ended up working on them at the same time, entangling their commits. That's my bad.

@santosjorge
Copy link
Owner

Thank you @jpoles1 for this. It seems there is a lot going on on this PR.

I see what you did with the violin charts, and is a problem I ran with before with the old candle charts (which were generated by the figure_factory before). These methods should not be called directly from iplot as it breaks the usage of the other arguments, as well as being able to switch to different themes.

Either these methods need to be called separately (as df.scatter_matrix does today) and not inside iplot, or alternatively the violin chart needs to be rewritten inside iplot so it can take into account the rest of the arguments.

That is why I previously suggested that I was going to give it a go with the distplot chart (only that I am on holidays, hence the delay). However happy if you want to point other people to your fork for this solution meanwhile.

Regarding the documentation - thank you for this. I was working with some guys that were developing a new doc generator that looked very promising, but I may go with Sphinx until then. Let me check the doc implementation you have here next week - and perhaps we can extract only that commit from this PR.

@IvoMerchiers
Copy link

@santosjorge any update on this? Because I think cufflinks would benefit greatly from proper documentation.

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.

Detailed Code Documentation Violin plot ?
3 participants