bugfix: creating patches with transform=None

[ChrisBeaumont]

In previous versions of matplotlib, passing transform=None to a patches.Patch object meant that the patch was specified in device coordinates (i.e., equivalent to transform=transforms.IdentityTransform()). This functionality seems to be broken at the moment. The following script demonstrates the problem; it should display a circle at data coordinates (2,2)

import matplotlib.pyplot as plt
from matplotlib.patches import Ellipse

ax = plt.gca()
ax.set_xlim((1,3))
ax.set_ylim((1,3))

#draw an ellipse over data coord (2,2) by specifying device coords
xy_data = (2, 2)
xy_pix = ax.transData.transform_point(xy_data)
e = Ellipse(xy_pix, width=100, height=100, transform=None)

ax.add_patch(e)
plt.show()

I don't know the best way to fix this, but the attached code is one solution.

[dmcdougall]

@ChrisBeaumont Thanks for pointing this out and submitting a pull request.

@pelson This makes the pickle test fail. Any ideas?

[pelson]

Firstly, thanks for pointing this out @ChrisBeaumont and then what is even better than that is that you have gone to the trouble of putting it all together in a convenient PR - very much appreciated!

I think this has been introduced by either 5df279d or 7d6fced (after git bisecting). The most likely change is the former, where the line in artist changed: 5df279d#L0R227 .

Your solution seems reasonable, although it does break one of the tests (the pickle test which you can run with python tests.py matplotlib.tests.test_pickle). The reason this has failed is that the streamline code is depending on the transData being put in for the arrow patches, which should be easy enough to fix.

There are likely to be other core artists which have this issue (collections spring to mind).

Given that your desired functionality is so simple to achieve (as you showed with the IdentityTransform), I wonder if we really want this behaviour by default. What compounds this is the fact that you are adding this artist to a specific Axes rather than the figure itself, which makes me think that the default behaviour of using data coordinates is a sensible one. I would love to get some feedback from other devs on this.

If we decide to go ahead with this change, then we should do it in the 1.2.x branch, as @ChrisBeaumont would be correct in calling this a bug fix.

Thanks!

[dmcdougall]

I agree with trying to get this into 1.2.x. I'll milestone it accordingly.

[ChrisBeaumont]

I'll let the experts sort out what the behavior of transform=None should be. I'll just add:

The "default" behavior (i.e. not specifying transform at all during instantiation) is to use data coordinates. I guess the question is whether transform=None should override the default or not.

The previous behavior of transform=None is "documented" to various extents on the web. For example, the transformations tutorial page indicates that None corresponds to the transform to display coordinates, and @pelson describes using transform=None in this SO post. So, if the behavior of transform=None will change in 1.2, it should probably be documented/advertised.

[dmcdougall]

Arguably, having transform=None correspond with display coordinates is also sensible. Changing it now, in my view, is a pretty non-trivial change. Considering, as @ChrisBeaumont points out, there appears to be avid web literature claiming display coordinates to be the default I'd say we keep display coordinates as the default for 1.2.

[dmcdougall]

This is milestoned for 1.2. Do we have any movement on this?

[pelson]

This is milestoned for 1.2. Do we have any movement on this?

My feeling is that, if we are going to fix it, we should fix it in 1.2.0.

[pelson]

What does the False do? My guess is that it doesn't need to be there.

[ChrisBeaumont]

.get() without a second argument returns None if the keyword is not found. We need to distinguish between a keyword not being supplied (kwargs.get('transform', False) returns False) and being explicitly set to None (returns None).

[dmcdougall]

In [1]: None is False
Out[1]: False

In [2]: None is True
Out[2]: False

[ChrisBeaumont]

I might be missing something, but this is what I was trying to avoid

In [1]: opts = {'transform' : None}
In [2]: if opts.get('transform') is None:
   ...:     print 'user set transform=None. Taking action'
   ...:     
user set transform=None. Taking action

In [3]: opts2 = {}

In [4]: if opts2.get('transform') is None:
    print 'user set transform=None. Taking action'
   ...:     
user set transform=None. Taking action

The desired behavior is

In [5]: if opts2.get('transform', False) is None:
    print 'user set transform=None. Taking action'
   ...:     

IOW, None is the desired True condition of get()

[pelson]

I'm comfortable that your code is doing what you intended, but I am not so sure that it is the correct behaviour. As I understand it, this would mean that:

PathPatch(path, transform=None)

and

PathPatch(path)

have different behaviour.

[ChrisBeaumont]

That's correct and (I believe) this is how matplotlib behaved previously. transform=None was different than specifying nothing, which fell back to data coordinates.

[dmcdougall]

As I understand it, this would mean that:

PathPatch(path, transform=None)
and
PathPatch(path)
have different behaviour.

This is where we need to agree or disagree. Having them behave differently is inconsistent with the rest of the codebase (I think).

[WeatherGod]

Nope, I think it is correct. In most places, an artist assumes data
coordinates. With only a few special exceptions, anytime you want your
transform to operate in non-data coordinates, you have to explicitly
provide that.

What is confusing is what the python None means. This is an example of
some of the inconsistencies that have developed over the years with
defaults. However, in this case, there is no rcParam to refer to. So long
as it is clearly documented what a transform of None means, I am fine with
this.

[dmcdougall]

This is an example of some of the inconsistencies that have developed over the years with defaults.

This is exactly what I was referring to.

However, in this case, there is no rcParam to refer to. So long as it is clearly documented what a transform of None means, I am fine with this.

Awesome. So None should mean "go to the rc param". If this is the case then since, as you point out, there is no rc param to default to, I am happy with this approach.

[pelson]

Ok. I'm satisfied that the behaviour is intentional (even if I am uncomfortable with it). I think it is worthy of a comment at this code point to mention that there is a different behaviour between None and not supplying anything. Other than that, I think this line of code is fine.

Thanks

[pelson]

I have one tiny concern with the implementation itself (commented inline). I would also like to see the equivalent fix to collections (I haven't tested, but assume the same issue applies). Additionally, the failing test will need updating also.

Other than that, thanks @ChrisBeaumont. 👍 +1

[dmcdougall]

I echo @pelson's comments. I would also like to add that the 'default' behaviour needs to be added to the documentation.

[mdboom]

What's left to be done on this?

[dmcdougall]

a) The documentation needs updating to improve clarity regarding default behaviour. It was decided that when the transform is not specified, the default should be to use data coordinates. To use display coordinates, one should pass transform=None.

b) If necessary, this fix needs to be applied to Collections.

c) @pelson has a concern that needs to be addressed (commented in-line).

[pelson]

c) @pelson has a concern that needs to be addressed (commented in-line).

That's correct and (I believe) this is how matplotlib behaved previously. transform=None was different than specifying nothing, which fell back to data coordinates.

I think this addresses it (not that I am particularly comfortable with the answer, but there is no reason to hold up the PR on that basis).

I will add one more thing to the todo list:

d) The failing test needs investigating and fixing.

@ChrisBeaumont - are you in a position to work on this today? If not, I think one of us will have to pick this up so that we can cut RC3 asap.

Thanks,

[dmcdougall]

@ChrisBeaumont - are you in a position to work on this today? If not, I think one of us will have to pick this up so that we can cut RC3 asap.

I can't pick this up today, I have my PhD viva tomorrow.

[pelson]

I can't pick this up today, I have my PhD viva tomorrow.

Best of luck with it Damon!

[ChrisBeaumont]

Good look @dmcdougall! Yes, since there seems to be agreement on what to do, I should be able to update this PR today.

[mdboom]

@dmcdougall: Yes! Best of luck! I hope matplotlib didn't suck too much of your preparation time away ;)

[dmcdougall]

Thank you all. Will report back tomorrow.

[ChrisBeaumont]

I tried addressing everyone's comments. In short:

1. I reverted the change that @pelson referred to (5df279d#L0R227), such that artist.set_transform(None) works as it did before

2. The danger here is that wrapper/factory methods which have a (transform=None) in their signature would force artists to use device coordinates. Unfortunately, that is frequently not the desired behavior. This was the problem with the streamplot test, which I've addressed

3. I added a new test

4. Unfortunately, the transform=None pattern is used by many of the transform classes, and it doesn't always mean "use device coordinates" (it often means "let the method figure out the default"). I've left that alone.

How does everyone feel about these changes? I'm afraid of breaking something -- have I?

[dmcdougall]

@pelson @ChrisBeaumont @mdboom passed! minor thesis corrections!

[dmcdougall]

Ok, there seems to be some mis-coordination with the Artists fix. It appears that ChrisBeaumont@26ebd62#L1L234 and pelson@0fce99a#L1L234 are addressing the same problem.

I prefer @pelson's tests. I think it's necessary to test both patches and collections.

Where do we go from here?

[pelson]

Ok, there seems to be some mis-coordination with the Artists fix.

Nope. I've been really short of time lately, but I wanted to get a fuller understanding of the problem that this PR addresses.
@ChrisBeaumont 's fix does fix the problem, but in doing so, adds a new meaning to the transform kwarg (False -> data coordinates if your on an Axes) - I'm not keen on None meaning one thing and False meaning another. I think I have a workable solution, but just haven't had the time to shape it into a complete PR just yet.

[pelson]

Ok. I have what I think is a viable alternative to this PR in #1431. @ChrisBeaumont : Given you wrote this PR and know the problem as well as anybody, your input on that PR would be massively valuable.

Cheers,

[ChrisBeaumont]

That certainly looks cleaner than what I came up with, so I'm in support #1431 over this. Some of the ugliness in that PR was in trying to put the transform=None functionality into factory methods like scatter and streamplot. Note that, in this PR, setting transform=None in those two methods still uses data coordinates.

Maybe that's fine. I noticed this bug because I had some code that drew patches based on mouse gestures (drawing a circle in, e.g., a log-log plot is easiest if no transform is used). I doubt that many people are creating scatter plots in device coordinates (and they still can if they need to by using IdentityTransform)

[mdboom]

Closed in favor of #1431