Skip to content
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

Artist autolim #28071

Closed
wants to merge 2 commits into from
Closed

Artist autolim #28071

wants to merge 2 commits into from

Conversation

jayyxiao
Copy link

@jayyxiao jayyxiao commented Apr 13, 2024

PR summary

Move auto-limit logic inside respective Artists

This will allow for relim to work with Collections. closes #25139 .

_update_*_limits() were moved inside respective Artists. An _in_autoscale flag was created in base Artist, with setters and getters. This flag can be a bool or tuple, as specified in #15595 . Also, _update_limits() was created in Collection. Thus, relim checks the _in_autoscale flag and calls a generic _update_limits(), which now exists in Collection.

_in_autoscale is set to True in the Artist constructor. Expansion to a tuple will be done by the setter. _in_autoscale can be set to False when theautolim parameter of add_collection is False.

Since I am a new contributor, I hope this can be a starting point. I'm just looking for some feedback.

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@jayyxiao jayyxiao closed this May 8, 2024
@tacaswell
Copy link
Member

@jayyxiao I'm sorry this got no attention when you opened this (I have been super busy the last month and know some of the other core developers are in a similar situation). I assure you our lack of response is in no way commentary on your work!

Giving it a quick skim this seems like it is heading the right direction, however I have one concern. Passing the Axes object into an artist method which then "returns" the results by mutating its parent is a bit odd. I think it would be better if the return from _update_limits was made uniform so the loop over children would look something like:

for artist in self.children:
    if not visible_only or (artist.get_visible() and artist.get_in_autoscale()):
        limits, to_update = artist._get_limits(self.transData)
        self.update_datalim(limits, **to_update)

Put another way, currently we have too much artist-specific stuff in Axes, but this swings too far the other way and puts too much Axes specific stuff into the Artists.

Additionally, being able to get the data limits of an artist is useful for other things (maybe cheaper hit-checking for picker, improving the auto-placement of the legend, etc) so being able to get at this might be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: generalize artists opting in-to auto limits
2 participants