Skip to content

Unexpected behavior for Axes.quiver(transform=None) #11153

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.

Sign up for GitHub

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

Jump to bottom
Closed
timhoffm opened this issue May 2, 2018 · 7 comments
Closed

Unexpected behavior for Axes.quiver(transform=None) #11153

timhoffm opened this issue May 2, 2018 · 7 comments
Labels
API: consistency

Comments

@timhoffm
Copy link
Member

timhoffm commented May 2, 2018

Bug report

Bug summary

I would expect that Axes.quiver(transform=None) would behave the same as leaving out the kwarg transform (None implying default). However this is not the case. Instead the default for a missing transform is Axes.transData.

As far as I understand, there is no reasonable semantics of an explicit transform=None here (except for 'use default'). It's probably an unintended implementation detail.

I therefore propose to change the behavior of Axes.quiver(transform=None) to be the same as leaving out the kwarg. This would allow further code simplification with explicit kwargs (#11145).

Code for reproduction

import matplotlib.pyplot as plt
import numpy as np

X = np.arange(-10, 10, 1)
Y = np.arange(-10, 10, 1)
U, V = np.meshgrid(X, Y)

fig, (ax1, ax2, ax3) = plt.subplots(1, 3, figsize=(12, 3))
ax1.quiver(X, Y, U, V)
ax2.quiver(X, Y, U, V, transform=None)
ax3.quiver(X, Y, U, V, transform=ax2.transData)

plt.show()

grafik

Expected: All there Axes should be equal.
@timhoffm timhoffm mentioned this issue May 2, 2018
Merged
2 tasks
@jklymak
Copy link
Member

jklymak commented May 2, 2018

I don't know: transform=None -> IdentityTransform throughout. I agree that your convention would be better, but thats not where we are at. Not sure why quiver would be special.

matplotlib/lib/matplotlib/artist.py

Lines 313 to 320 in 77ca117

def get_transform(self):
"""
Return the :class:`~matplotlib.transforms.Transform`
instance used by this artist.
"""
if self._transform is None:
self._transform = IdentityTransform()
elif (not isinstance(self._transform, Transform)

@ImportanceOfBeingErnest
Copy link
Member

I find the semantics of property=None meaning "no such property" much more understandable than "use default property". E.g. facecolor=None resulting in "no facecolor" is semantically correct; while the current result "use default facecolor" is not.

Because there has been a decision at some point to break the natural semantics and use the mapping None -> "default", it might make sense to apply this throughout the library, also for transform. But I suppose this would break some existing code and if being done needs to be consistent throughout the whole library.

@timhoffm
Copy link
Member Author

timhoffm commented May 2, 2018

I find the semantics of property=None meaning "no such property" much more understandable than "use default property".

However, we use it almost everywhere as "use default property". Some places use the string 'none' to mark "no such property".

I suppose the affected user code will be little. You have to explicitly use transform=None and at least in my example this didn't generate any reasonable output.

@ImportanceOfBeingErnest
Copy link
Member

Yes, in this case it would probably be ok and wouldn't break anything. But if you only change it here, things become even more inconsistent. There are other cases where the default is None, and you would need to check them all not to break anything.
I do know that for the axes_grid1.inset_locator.inset_axes the default is None, and it needs to be None in the sense of "no transform", in order for the default cases to work. A naive attempt to change that (#10756) resulted in big chaos and was a lot of work to understand and fix (#11060 - this still needs a review btw. so in order to dive into the topic it might be a good start 😃).

@efiring
Copy link
Member

efiring commented Jul 2, 2018

I favor @timhoffm's proposal. None as 'use the default' is deeply embedded and works well. Where we mean 'nothing' we use the string, 'none'. 'axes_grid1', or anything else in mpl_toolkits, should not be used as a model for matplotlib in general. There is a reason those things are in toolkits.

@ImportanceOfBeingErnest
Copy link
Member

I favor @timhoffm's proposal. None as 'use the default' is deeply embedded and works well.

That may be true for most other cases, but not for transform (as commented twice about already). So you should not make None the default transform, unless you do it throughout the library.
(@efiring Feel free to ignore my example from toolkits, just consider the matplotlib.artist.Artist and everything that is derived from it, if you want. That example was only meant to show that if you naively do this change, you may easily introduce bugs, which may then be hard to track down.)

@efiring
Copy link
Member

efiring commented Jul 2, 2018

OK, after a discussion with @jklymak I have decided to close this, recommending that Quiver be left as-is in this respect. I note that quiver does not expose transform as an explicit kwarg but merely passes it through via **kw. That should reduce or eliminate the expectation that setting it to None will yield its Quiver default value.

@efiring efiring closed this as completed Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
API: consistency
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@efiring @jklymak @timhoffm @ImportanceOfBeingErnest