Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Use kwonlyargs instead of popping from kwargs#11145
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| self.barb_increments=barb_incrementsordict() | ||
| self.rounding=rounding | ||
| self.flip=flip_barb | ||
| transform=kw.pop('transform',ax.transData) |
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.
Is there a reason not to converttransform?
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.
IfNone could be a user-supplied value, yes. Otherwise no!
For this pull I'm really trying to make a small, obviously correct, incremental step. I can't clean it up all at once, and I'm not going to try - I'd go crazy before it was debugged, let alone reviewed...
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 see the value of small incremental steps and agree with the strategy.
Indeed, explicitly providingtransform=None is currently accepted and results in anIdentityTransform. I suppose this is not intended, but should be discussed as a separate issue.
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 think None as IdentityTransform is fine...
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.
@anntzer I don't think so, because if not given, the default is ax.transData and in a quick test, I didn't get any reasonable output when usingquiver(..., transform=None) explicitly. But let's not go there in this PR. I'll open an issue for this soon when I have time to write it up correctly.
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.
See#11153
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.
... all artists have a default of transform=None maps to IdentityTransform. Ummm, why? I have no idea. Back in the day, I think more was done in pixel co-ordinates.
| vmin=kwargs.pop('vmin',None) | ||
| vmax=kwargs.pop('vmax',None) | ||
| linewidth=kwargs.get('linewidth',None) | ||
| shade=kwargs.pop('shade',cmapisNone) |
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.
shade could be converted as well (you already did it for plot_trisurf 😄)
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've actually reversed that now in an attempt to eliminate all behaviour differences 😭
(my mantra: be incremental; you can come 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.
An explicitshade=None was interpreted asshade=False. I suppose this is also an unintended implementation detail. I'm happy to discuss this separately.
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.
See#11154.
2269e22 toed13662Compare
timhoffm left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Also for some reason, at least some of the generated test images are not antialiased, where the expected images are. Did you change something in this respect?
lib/matplotlib/quiver.py Outdated
| self.coord=kw.pop('coordinates','axes') | ||
| self.color=kw.pop('color',None) | ||
| self.angle=angle | ||
| self.coord=coord |
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.
Unwanted name changecoord /coordinates
Zac-HD commentedMay 2, 2018
Yep, I had a typo in |
Zac-HD commentedMay 2, 2018
🎉@timhoffm, it's all working now. Ready to merge? |
jklymak left a comment
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.
Thanks for this - I think these are all very helpful and will make the signatures more informative!
This pull contains part of my follow-up to#11137, likewise addressing#9912.
For this one, I've only included the cases where a straight translation from kwargs.pop to keyword-only arguments gives exactly the same behaviour (modulo two cases where unknown kwargs now raise TypeError!). There are many more that require some additional refactoring, but in the hope of a quick review cycle I'm leaving them all for later.