-
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
Merged
jenshnielsen
merged 8 commits into
microsoft:master
from
nulinspiratie:feature/matplot_upgrades
Jun 21, 2017
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
0d131ea
Copied MatPlot changes from Nulinspiratie fork
nulinspiratie 341bc39
fix: Forgot a call to _get_axes
nulinspiratie 9724344
refactor: made subplot kwarg 1-based
nulinspiratie 8009e06
refactor: made default_figsize static, remove trailing white spaces
nulinspiratie 04c5a1b
fix: kwargs are also passed if there are multiple args provided.
nulinspiratie 0ea2525
fix: also allow multiple subargs as first arg
nulinspiratie d8abdec
fix: forgot to change default subplot=1 in update_plot
nulinspiratie 35da15f
Merge branch 'master' into feature/matplot_upgrades
jenshnielsen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 aboveThere 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 infig, axes=pyplot.subplots()
being an array, and hence 0-based. I hope this doesn't add more confusion thoughThere 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