-
Notifications
You must be signed in to change notification settings - Fork 6
improved .plot methods #925
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
Conversation
Yes, this must be done in the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions and suggestions.
The main suggestion would be to update the setup.cfg file to avoid to add # doctest : +SKIP
everywhere in the Array.plot() examples.
LGTM by the way. |
…and use it) within larray
c507f74
to
d5894aa
Compare
@alixdamman: I thought it would take me 2-3 hours to finish this, so that it can be included in the next release of larray and I can work on LIAM2 without interruption but it took me 5 days 😭. If I had known, I wouldn't have done it now but... so is life 🤷♂️. |
@alixdamman: the review I am asking now is not about the code, but I'd be grateful if you could play around with it, to see if there are more bugs I didn't find. You could also check the release notes to see if it is understandable... |
PS: sorry for the rebase-force-push but the code changed so much since the first version it did not make sense to make commits "from there" |
If I may, the improvement of the plot function comes from a specific demand or your wish to encourage our users to stop using Excel for everything ? (there is no wrong answer) |
There wasn't any specific demand (hence the remark that I wouldn't have done it now had I known it would take me that much time). FWIW, the subplots part is something I always found frustrating, and the rest are things I found while developing the subplots part. But this might help a bit with a vague demand I regularly receive of "better plotting tools". I never investiguated what each of these demands really mean. I just know bm wants to easily do more kinds of visualisations so that wouldn't qualify, but I think other members of the IO team also use misc Python visualization to explore their data, so this PR will probably help streamline that kind of workflow. |
Also, a few improvements in there could be considered bug fixes as many plot submethods were not really usable as is and needed very convoluted ways to set them up including transpose, combine axes and explicit for loops to get something decent, so that's no wonder nobody used them 😄. |
* support for subplots=axes, x=axes, y=axes and by=axes in plot functions * support for labels (instead of axes) in x and y for line plot and scatter * support legend kwargs * better title when subplots is True * default subplots layout to last axis horizontal * pie legend defaults to False (it does not bring any more information than there already is)
d5894aa
to
a7b0074
Compare
I know the tests will fail because of the bogus Pandas doctest for plot.line but I would
like your advice on how to solve this. Do you know of a way in pytest to ignore one particular method doctests?