Use kwonlyargs instead of popping from kwargs #11145
Conversation
| self.fill_empty = fill_empty | ||
| self.barb_increments = barb_increments or dict() | ||
| self.rounding = rounding | ||
| self.flip = flip_barb | ||
| transform = kw.pop('transform', ax.transData) |
There was a problem hiding this comment.
Is there a reason not to convert transform?
There was a problem hiding this comment.
If None 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.
I see the value of small incremental steps and agree with the strategy.
Indeed, explicitly providing transform=None is currently accepted and results in an IdentityTransform. I suppose this is not intended, but should be discussed as a separate issue.
There was a problem hiding this comment.
I think None as IdentityTransform is fine...
There was a problem hiding this comment.
@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 using quiver(..., 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.
... 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.
| norm = kwargs.pop('norm', None) | ||
| vmin = kwargs.pop('vmin', None) | ||
| vmax = kwargs.pop('vmax', None) | ||
| linewidth = kwargs.get('linewidth', None) | ||
| shade = kwargs.pop('shade', cmap is None) |
There was a problem hiding this comment.
shade could be converted as well (you already did it for plot_trisurf 😄)
There was a problem hiding this comment.
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.
An explicit shade=None was interpreted as shade=False. I suppose this is also an unintended implementation detail. I'm happy to discuss this separately.
2269e22 to
ed13662
Compare
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.
Unwanted name change coord / coordinates
Yep, I had a typo in |
|
🎉 @timhoffm, it's all working now. Ready to merge? |
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.