matplotlib icon indicating copy to clipboard operation
matplotlib copied to clipboard

Document idiosyncratic behavior of `transform` kwarg

Open munkm opened this issue 6 years ago • 12 comments

Bug report

Bug summary

Passing transform=None through imshow results in a blank plot.

I've been playing around a little bit with data transforms and projections in matplotlib. For one of the things I've been testing out, I also want to see how my data looks if the transform isn't set by the user, so I passed in transform=None as a kwarg to imshow. This resulted in a blank plot. I'm not sure if this is a preferred outcome, but I personally would have expected a plot without a transform set, but the data showing.

Code for reproduction

from numpy import random
import matplotlib.pyplot as plt

data = random.random((20,20))

fig, (ax1, ax2)  = plt.subplots(2)

ax1.imshow(data)
ax2.imshow(data, transform=None)
plt.show()

Outcome image

Matplotlib version

  • Operating system: OSX Sierra 10.12.6
  • Matplotlib version: 2.2.2 and 3.0.0
  • Matplotlib backend (print(matplotlib.get_backend())): MacOSX
  • Python version: 3.6.5 and 3.6.6

I tried testing this script out in two different conda environments that I have on my machine with the same result. Both envs I tested this script out on use matplotlib pulled from conda-forge. One environment has v2.2.2 and the other has v3.0.0

munkm avatar Oct 17 '18 21:10 munkm

The image is clipped by the axes. You will see it if you make your canvas squared and remove any margins.

from numpy import random
import matplotlib.pyplot as plt

data = random.random((20,20))

fig, ax = plt.subplots(figsize=(2,2))
fig.subplots_adjust(0,0,1,1)

im = ax.imshow(data, transform=None)
plt.show()

imwithouttrafo

I currently have no idea why clip_on=False would not show it otherwise as well; there might be something special about images in this respect.

I will close this as there does not seem to be any bug involved and transform=None does not seem like a use case that needs to be supported. Yet, if you feel that there is a need for this to work, please feel free to reopen.

ImportanceOfBeingErnest avatar Oct 17 '18 22:10 ImportanceOfBeingErnest

Why is it that if I pass transform=None to imshow that radically changes the plot? This makes it a lot harder to write code that generically deals with transforms or cases where sometimes there might be a transform and sometimes not. I've been working on a feature for yt (which is linked above) and this seems like pretty reasonable behavior to expect.

munkm avatar Oct 17 '18 22:10 munkm

Because that's what transform=None (which is interpreted as =IdentityTransform()) means. The transform indicates how to convert from data coordinates to screen (pixel) coordinates; when using IdentityTransform this literally means "the image data maps to the raw pixels".

You likely want to use transform=ax.transAxes instead, see https://matplotlib.org/tutorials/advanced/transforms_tutorial.html#sphx-glr-tutorials-advanced-transforms-tutorial-py for more details.

anntzer avatar Oct 17 '18 22:10 anntzer

Thank you @anntzer. That's helpful and I appreciate your explanation.

munkm avatar Oct 17 '18 23:10 munkm

The confusion is absolutely understandable. This is a semantic pitfall, unfortunately anchored deeply inside matplotlib. On the one hand, argument=None is mostly interpreted as "use the default", e.g.

  • imshow(..., cmap=None) ==> use the default colormap (the one from rcParams["image.cmap"])

on the other hand it is sometimes interpreted in the literal sense, "no such thing"

  • imshow(..., transform=None) ==> do not use any transform (i.e. the IdentityTransform)

ImportanceOfBeingErnest avatar Oct 17 '18 23:10 ImportanceOfBeingErnest

The underlying issue here is how we handle kwargs, what 'default' means, and when we decide what it is.

In the draw tree each artist is responsible for knowing what transform to use for it's data. This draw tree and and transforms are both hierarchical in the sense that the Figure objects contains Axes objects which contains Axis and all of the actual data Artists as children and the transform object on the Axes typically has a reference to the transform on the Figure and the data artists typically have a reference to the Axes transform (+ the scales, this is how changing from linear to log works for example and how moving where an Axes is placed in a Figure propagates down to tell the line which pixel it should color). However we also allow users to set what ever transform they want on the artists (this is super useful for things like putting text at a fixed location on an Axes so it does not move as you change the data rate or scale). Additionally, when we create the Artists instances, they do not yet know what Axes it will be put into so we start with the _transform attribute being None. The public interface to getting the transform of an artist is get_transform. To keep type stability, the accessor return IdentityTransform (which makes sense, you didn't tell us the transform so we go with the identity as the default). When we add the Artist to a parent (a Figure or an Axes) we set the transform on the new child from the new parent. However, we don't want to do this if the user has explicitly set the transform so we also track if set_transform has been called or not and if it has not been called, we do not set the transfrom. Calling art.set_transform(None) sets the the _transfrom attribute to None which restores the default behavior.

So for so good? I think all of these make good sense and have decent semantics. The edge case here is the way we handle **kwargs. Almost all of our function take **kwargs and pass them through to the next layer down. In the case of most the Artists we eventually call self.update(kwargs) in the __init__ methods which more-or less does [getattr(self, f'set_{k}')(v) for k, v in kwargs.items()]. Thus, if you pass in transform=None as a keyword argument then the transform on the artist is set to the default value, but the internal state of the 'did the user explicitly set the transform' flag is flipped so that the when the image artist gets added to the Axes, the transform does not get reset.

We could filter to ignore transform if it is None, but it is not clear where the right place to do that is. Doing it in Artist.update is probably not the right place (as it is part of the public API and we don't want to filter-out valid user input there) and if we do it sub-class level then we could end up in a worse situation where sometimes we have this odd behavior of transform and sometimes not.

This is one of the more idiosyncratic behaviors of the library (as you can tell from my small novella attempting to explain it), but this is unfortunately the expected behavior and I don't think it can be changed.

@munkm I'm sorry that the initial comments on this issue were short. This should be documented (where would you have expected to find this information? maybe in the transform tutorial? On every Aritst subclass seems over-kill). I have taken the liberty of re-naming the issue and re-opening it.

tacaswell avatar Oct 18 '18 22:10 tacaswell

Actually, now that I'm reading this (nice) writeup, I guess a better question is whether we want None to mean IdentityTransform() in the general case. If we decide this behavior is too confusing, we can also just make set_transform() not accept None as input at all. Then the fact that we internally represent an unset transform with None would just be an implementation detail (we can then get rid of the additional, slightly confusing _transformSet flag, which becomes redundant), and we can even make .get_transform() raise an exception if the transform is currently unset. I think the proposed change can easily go through a deprecation path, it's just a question of whether the semantics are sufficiently better/clearer to be worth it.

anntzer avatar Oct 19 '18 08:10 anntzer

If we deprecated set_transform taking None it would change the OP in to raising a confusing error instead of silently having confusing behavior which does seem like an improvement.

What do people think about "conditional deprecation" like "we want to deprecate this so we are warning, but if this is going to be a problem for you let us know so we can re-consider". I think this would help to mitigate the risk of the unknown user base of some of the weird corners of the library.

tacaswell avatar Oct 19 '18 17:10 tacaswell

The error doesn't even have to be confusing, we can word it appropriately. I'd just put in a normal deprecation warning and hopefully someone will complain if they're not happy with it, at which point we can reconsider.

anntzer avatar Oct 19 '18 17:10 anntzer

The transforms tutorial is pretty clear that None is the same as the Identity.

image

It also says

it is most common for artists placed in an axes or figure to have their transform set to something other than the IdentityTransform(); the default when an artist is placed on an axes using add_artist is for the transform to be ax.transData.

In contrast, the set_transform must have been written by a purist

image

and could sure get some more detailed description.

In general, a problem is that kwargs are not (well) documented. In imshow it says

Other Parameters: | **kwargs : Artist properties image

It's not actually clear what "Artist properties" are. It requires some experience to guess that everything that has a set_ in front of it may potentially be used as an argument. Clicking on AxesImage is a dead end, as that inherits from a private undocumented base class.

So foremost the confusion is really that transform=... isn't documented anywhere (at least not specifically for imshow).

In addition, some functions take a bbox_transform argument, which could be easily confused with the transform of the Artist. e.g. legend. Here, the behaviour is different,

bbox_transform : None or matplotlib.transforms.Transform The transform for the bounding box (bbox_to_anchor). For a value of None (default) the Axes' transAxes transform will be used.

None will default to the transAxes. But it is clearly documented.

For axes_grid1.inset_locator.inset_axes the behaviour is again different,

bbox_transform : matplotlib.transforms.Transform, optional Transformation for the bbox that contains the inset axes. If None, a transforms.IdentityTransform is used.

None will default to the IdentityTransform. But it is clearly documented.

All of this to say: Maybe solving the "confusion" problem is best be solved on a documentation level. And changes in the code base will most probably lead to more confusion, not less.

ImportanceOfBeingErnest avatar Oct 19 '18 19:10 ImportanceOfBeingErnest

Thank you to all of you for the thoughtful commentary on here. The discussion has certainly enlightened me to a lot more of matplotlib's functionality, and I really appreciate you all taking the time to explain all of this to me. 🙂

I will say that before I filed my issue I did look at the documentation and I also cloned and perused some of the artist source because I didn't want to file a non-issue issue here. These are the documentation pages I was looking at: the transforms tutorial the transforms api page dev guide on creating scales and transformations

In the source I looked through artist.py and axes/_axes.py. I did originally try to look at the documentation for imshow() to learn about the defaults for the transform= kwarg but didn't find anything there.

When I saw weird behavior in my function by passing transform=None into imshow() I looked at the init of Artist and saw that self._transform=None. I assumed that this was the default behavior of the function and that if I passed None I would have the same behavior. I also perused some of axes/_axes.py, but started confusing myself by seeing some one place where

if transform is None: 
transform = self.transAxes

and another where

if transform is None: 
transform = self.transData

and in Artist of artist.py

if self._transform is None: 
self.transform = IdentityTransform()

so I wasn't sure which was the default if I didn't set the transform.

Admittedly, I didn't read the documentation as thoroughly as I should have, because I probably would have ended up figuring out that (a) the identity transform would skew the image and I wouldn't be able to see it, and (b) that setting None is the same as setting the IdentityTransform and that is not the default behavior.

As mentioned above, the default behavior is documented in the transforms tutorial as

the default when an artist is placed on an axes using add_artist is for the transform to be ax.transData

further, None being the same as the IdentityTransform() is also documented on the transforms tutorial page.

However, even though I read this particular page multiple times I missed the sentence about the default behavior, maybe because at the end of a second paragraph below the table of transforms. I think

  • making the default behavior clearer (for example, in the table at the top of the Transformation tutorial, as None is) and perhaps in multiple places would have been helpful for me.
  • it would be helpful to know how **kwargs are handled also, since I didn't realize that they were updated in the way that you described.
  • understanding this is really helpful, but I'm not sure of where to document it:

Thus, if you pass in transform=None as a keyword argument then the transform on the artist is set to the default value, but the internal state of the 'did the user explicitly set the transform' flag is flipped so that the when the image artist gets added to the Axes, the transform does not get reset.

I am not sure of the best place to put any of this information (though the transformations tutorial does sound like a good place), but I hope that I helped by describing how and where I went wrong in trying to figure out the issue.

munkm avatar Oct 19 '18 21:10 munkm

Having None mean "the default" is useful when that default is not easily specifiable otherwise (e.g., when it is (derived from) an rcParam). When the default is IdentityTransform(), I don't think there's much value in supporting None as a synonym -- especially when a similarly named argument (bbox_transform) has a different default! In other words, having transform=None and bbox_transform=None mean different things (and, in fact, bbox_transform=None mean two different things depending on the class) doesn't seem worth it.

Hence I'd still suggest getting rid of accepting None there (again this doesn't affect whether we internally use None to represent an unset value); or, at least, fix the defaults so that they are consistent.

anntzer avatar Oct 20 '18 10:10 anntzer