Skip to content

Deprecate offset_position="data". #13696

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 18, 2019

The offset_position property of collections is currently set to "data"
only for hexbin(), but one can do away with it by just setting the
correct offsetTrans instead -- which this PR does. The advantage of
doing so is that the correct offsetTrans only needs to be implemented
once, whereas support for offset_position needs to be implemented for
each renderer class separately (including at the C-level for the Agg
backend).

Note that the offset_position argument to draw_path_collection is
not pending removal because its removal would require coordination with
third-party backends; the plan is to just always set it to "screen"
after the deprecation period.

If there is demand for it, _VectorTransform could be made a public API.


TLDR: hexbin() currently (effectively) uses its own code-path in renderer code, but that's not needed.

Edit: closes #16461 and (effectively) #16496

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@ImportanceOfBeingErnest
Copy link
Member

In general, this makes sense to me.

Is there anything that would speak against making VectorTransform public? That would allow to directly let users know what to use in lieu of the deprecated "data" argument, without the need to reinterate the complete thing in case someone does have a custom use case of it.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 20, 2019

I would strongly doubt that there are many users of offset_position="data". But if you're worried about this we can replace the Deprecation by a PendingDeprecation, as usual. (On the other hand, seeing whether we get complaints or not is also one of the points of the deprecation period.)
At least I haven't fully thought about what the public API re: VectorTransform should be, and I would perhaps like to use something similar to replace some of the annotation coord strings (cf discussion at #13515).

@ImportanceOfBeingErnest
Copy link
Member

Even if the amount of users hit by this isn't massive, it's still not the right policy to first break something and then wait if someone complains. Often enough, people might just be frustrated without opening an issue about it. In this case I think that the functionality of an offset transform in data coordinates is in general useful. I would agree that this does not require an additional argument, but should be incorporated into the transform itself. So removing offset_position in the long run makes sense. But if no alternative can be provided by now, the whole thing can be postponed up to when a general concept for such offset transforms is developed.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 24, 2019

The changelog entry can mention e.g. "if you need this functionality, you can vendor the implementation of _VectorTransform" (which is quite short if you don't bother with __str__) "Please let us know if that's the case; then we will consider making this a public API". (If one is really using offset_transform="data", that's a fairly low level thing and vendoring the transform code is really not that hard once you already have to update the call sites anyways.)

I simply disagree that we can never deprecate things on the basis that someone may possibly be using them; as always there is a cost-benefit ratio that needs to be considered, and certainly if someone actually complains about it we'll consider it more.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 26, 2019

(Some notes mostly to self: there's already transforms.offset_copy and text.OffsetFrom on the same theme.)

@anntzer anntzer force-pushed the offset_position-data branch from 8cba46e to 87a5d54 Compare March 26, 2019 18:20
@tacaswell tacaswell added this to the v3.2.0 milestone Mar 29, 2019
@efiring
Copy link
Member

efiring commented May 22, 2019

@anntzer If I understand correctly, hexbin could be implemented without offset_position or the new _VectorTransform. Would such an implementation be inferior? Slower, for example?
The transforms machinery has always been hard to understand, visualize, and remember, at least for me, and it would be good if any changes that are made serve to make it a little less difficult, a little more readable. The name _VectorTransform is not helping.

@anntzer
Copy link
Contributor Author

anntzer commented May 22, 2019

The problem is that without _VectorTransform (I'm not particularly in love with the name), collections (or more specifically, _CollectionWithSizes, which PolyCollection (the class returned by hexbin()) inherits) will by default size their components (the hexagons, in this case) to have a size in physical units (points), whereas we want to have them in data units, which is what _VectorTransform achieves. (Previously, this had to be built in to every renderer class, _VectorTransform just factors out that functionality.)

We could instead use PatchCollection, which just takes a bunch of patches and plots them as is (see e.g. the very recent thread on the mailing list https://mail.python.org/pipermail/matplotlib-users/2019-May/001663.html); however such an approach would at least lead to more inefficient vector output because the vector file would need to include every hexagon on every point, rather than a single hexagon and encode a list of offsets. In fact, this is the very reason why offset_position exists (#859); that PR indicates that just dropping the amount of data sent from python to agg also significantly accelerates agg.

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Sep 1, 2019
@tacaswell
Copy link
Member

Pushing out to 3.3 as I think this needs to be understood a bit better still.

It also looks like we currently have the ability to cycle through data transfroms (?!) and there is no documented path for a user who was directly creating the collection and passing data to adjust their code.

@ImportanceOfBeingErnest
Copy link
Member

I would like to reiterate the possibility to make _VectorTransform public. There has just been #16548, where of course one could draw circles in data coordinates, but in addition to the options shown in the SO post OP linked to, probably the fastest option would be to use a PathCollection.

import numpy as np
import matplotlib.pyplot as plt
import matplotlib.collections as mcoll
import matplotlib.transforms as mtrans
import matplotlib.markers as mmarkers

c = mcoll.PathCollection(
                [mmarkers.MarkerStyle("o").get_path()],
                offsets=np.repeat(np.arange(10),2).reshape(10,2),
                edgecolors=["k"]*10,
                transOffset=mtrans.IdentityTransform(),
                offset_position="data")
plt.gca().add_collection(c)
plt.gca().set(xlim=(-2, 11), ylim=(-2,11))
plt.gca().set_aspect("equal")
plt.show()

That option is gone if offset_position="data" is deprecated without replacement.

@anntzer anntzer force-pushed the offset_position-data branch from 9ebea41 to 791f09f Compare February 19, 2020 13:09
@anntzer
Copy link
Contributor Author

anntzer commented Feb 19, 2020

OK, I made VectorTransform public, but marked it as experimental to allow for later changes depending on actual use..

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo my question about more agressively calling _unstale_viewLim.

@@ -2612,6 +2612,26 @@ def get_matrix(self):
return self._mtx


class VectorTransform(Affine2DBase):
Copy link
Member

@timhoffm timhoffm Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I‘m not quite happy with the name. „Vector“ has a lot of semantics, but IMHO only a loose connection to „no offset“. Some possible alternatives:

  • WithoutOffsetTransform
  • RotationOnlyTransform
  • IgnoreOffsetTransfrom

Possibly even with an ...Wrapper suffix to indicate that this modifies / builds upon another transform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked vector as it's meant to transform the delta vector between a pair of points, rather than individual points (I agree this may not be obvious). Ignore/WithoutOffset seems to hide the relevant semantics to me and focus more on how they are achieved, and RotationOnly is wrong as there can also be stretch.
I'm open to changing the name, though.

@timhoffm
Copy link
Member

timhoffm commented Mar 2, 2020

Ignore/WithoutOffset seems to hide the relevant semantics to me and focus more on how they are achieved

Can you paraphrase that semantics (maybe even directly in the docstring)? I think I probably haven't fully understood offset_position='data' and VectorTransform, but maybe that's also the point. The current docstring just says

A transform wrapper that ignores the offset component of a base transform.

And that's what I've been trying to pick up for the name. That description and the name VectorTransform do not make a connection in my head.

@anntzer anntzer force-pushed the offset_position-data branch 2 times, most recently from 459461c to 234c905 Compare March 2, 2020 20:48
@anntzer
Copy link
Contributor Author

anntzer commented Mar 2, 2020

Edited the docstring, does this look better?

@timhoffm
Copy link
Member

timhoffm commented Mar 4, 2020

Thanks, the extended description looks good. I still feel a mismatch between the class name and the summary line. IMHO they should be on the same level of abstraction. So either

class VectorTransform(Affine2DBase):
    """
    A transform for connection vectors between points.

    This wraps another transform and ignores its offset component.
    ....

or

class IgnoreOffsetTransfrom(Affine2DBase):
    """
    A transform wrapper that ignores the offset component of a base transform.
    ....

I think that, more precisely, the vectors you think of are direction vectors (in contrast to position vectors). Maybe DirectionVectorTransform is a suitable alternative name?

@anntzer anntzer force-pushed the offset_position-data branch from 234c905 to 5d07a88 Compare March 4, 2020 15:09
@anntzer
Copy link
Contributor Author

anntzer commented Mar 4, 2020

Ah, I see the confusion now. Yes, I meant https://en.wikipedia.org/wiki/Displacement_(geometry), not https://en.wikipedia.org/wiki/Position_(geometry).

Pushed a new version.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring is good now. I would like to have a second opinion on the name VectorTransform, which I still find too broad.

@efiring
Copy link
Member

efiring commented Mar 6, 2020

DifferenceTransform? DeltaTransform? SizeAndShapeTransform? I'm struggling. It would really be nice to have a name that would be highly evocative--which for me, VectorTransform still is not. It seems that the zeroing of the offset really is the key to understanding what the wrapper is doing. So I tend to favor ZeroOffsetTransform, or NoOffsetTransform, or something like that. Or, if you want to include the mathematical property, it would be DistributiveTransform, since it is modifying a transform to make it distributive with respect to addition.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 6, 2020

DisplacementTransform? OffsetTransform?
Or we can just poll this over the call :)

@timhoffm
Copy link
Member

timhoffm commented Mar 6, 2020

Let‘s briefly discuss in the call.

@QuLogic
Copy link
Member

QuLogic commented Mar 23, 2020

I don't believe this came up in any call; I've put it on the agenda for next week (though we might not get to it.)

@tacaswell
Copy link
Member

On call we settled on AffineDeltaTransfrom for name.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 13, 2020

renamed

@anntzer anntzer force-pushed the offset_position-data branch from 5d07a88 to df5db91 Compare April 13, 2020 20:18
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bonus points if you have an idea how to sneak the word „delta“ into the docstring. That would make the name even more clear (I know that you‘ve not chosen it). 😄

@anntzer anntzer force-pushed the offset_position-data branch from df5db91 to 4728c32 Compare April 13, 2020 20:49
@anntzer
Copy link
Contributor Author

anntzer commented Apr 13, 2020

done, as "position deltas".

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone can merge on CI green.

The `offset_position` property of collections is currently set to "data"
only for hexbin(), but one can do away with it by just setting the
correct offsetTrans instead -- which this PR does.  The advantage of
doing so is that the correct offsetTrans only needs to be implemented
once, whereas support for `offset_position` needs to be implemented for
each renderer class separately (including at the C-level for the Agg
backend).

Note that the *offset_position* argument to `draw_path_collection` is
not pending removal because its removal would require coordination with
third-party backends; the plan is to just always set it to "screen"
after the deprecation period.

AffineDeltaTransform can be used as a replacement for
`offset_position="data"`.  This API is experimental.
@anntzer
Copy link
Contributor Author

anntzer commented Apr 27, 2020

rebased

@QuLogic
Copy link
Member

QuLogic commented Apr 27, 2020

Travis reporting seems broken; there are not currently any builds running.

@QuLogic QuLogic merged commit a4378ef into matplotlib:master Apr 27, 2020
@anntzer anntzer deleted the offset_position-data branch April 27, 2020 22:18
@anntzer anntzer mentioned this pull request Jul 24, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hexbin if singular and mincnt used
6 participants