-
Notifications
You must be signed in to change notification settings - Fork 315
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
feat: MatPlot updates (Updated version of #337) #636
feat: MatPlot updates (Updated version of #337) #636
Conversation
|
||
kwargs: with the following exceptions (mostly the data!), these are | ||
passed directly to the matplotlib plotting routine. | ||
`subplot`: the 1-based axes number to append to (default 1) |
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.
The default has changed from 1 to 0 and this is now zero based. Is that intentional?
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.
Ah sorry I forgot to mention that. I made that change quite a while ago, but yes it was intentional. I couldn't understand why subplots started at 1, since Python is 0-based. Combining it with subplot indices (plot[idx]
), it made more sense to me to use 0-based axes numbers. Do you agree? If so, I could modify the docstring above
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.
In general I agree that it's most logical for all arrays to be 0 indexed. I think the reasoning here is that pyplot.subplot(nml) is 1 indexed, as in pyplot.subplot(211) gives the first subplot of the 2,1 grid. That is an artefact of matplotlibs api being inspired by Matlab.
The biggest worry is that this will break backwards compatibility, @QCoDeS/core any opinion on this?
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.
Ah right good point. In that case we could opt for the subplot kwarg to still be 1-based, while the indexing to be 0-based, similar to pyplot.add_subplot
being 1-based, but the axes in fig, axes=pyplot.subplots()
being an array, and hence 0-based. I hope this doesn't add more confusion though
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.
Yes that is probably the best even if a bit confusing. It's consistent with matplotlib any way
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.
I would also opt for keeping consistency with matplotlib. That way I reckon we confuse the smallest amount of people to the least degree (although confusion is inescapable).
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.
Alright just added a commit that changed this
Author: Serwan Asaad <[email protected]> feat: MatPlot updates (Updated version of #337) (#636)
* Copied MatPlot changes from Nulinspiratie fork * fix: Forgot a call to _get_axes * refactor: made subplot kwarg 1-based * refactor: made default_figsize static, remove trailing white spaces * fix: kwargs are also passed if there are multiple args provided. * fix: also allow multiple subargs as first arg * fix: forgot to change default subplot=1 in update_plot
* Copied MatPlot changes from Nulinspiratie fork * fix: Forgot a call to _get_axes * refactor: made subplot kwarg 1-based * refactor: made default_figsize static, remove trailing white spaces * fix: kwargs are also passed if there are multiple args provided. * fix: also allow multiple subargs as first arg * fix: forgot to change default subplot=1 in update_plot
Fixes #257, and replaces #337 since that branch was deleted. Also includes fixes proposed in #615.
Changes proposed in this pull request:
MatPlot(data.arr1, data.arr2)
will create two subplots, one for each arr. An arg can also consist of multiple args, in which case they will all be plotted on the same subplot (e.g. MatPlot([data.arr1, data.arr2], data.arr3)` will create two subplots: one for arr1 and arr2, the other for arr3).plot[k]
is now identical toplot.subplots[k]
). Theadd
method is furthermore added to each subplot, meaning that you can now writeplot[k].add(data)
, which is identical toplot.add(data, subplot=k)
. A consequence is that the method_get_axes
is not needed anymore, and is therefore removed.subplots
kwarg, it will usesubplots=(1,3)
by default. For 6 args, it will becomesubplots=(2,3)
.default_figsize()
returns a default figsize to use depending on the shape of subplots.tight_layout()
, which performsfig.tight_layout()
with some extra space at the top for the title. It is called at the end of initialization@giulioungaretti @jenshnielsen