Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add set_in_autoscale() method#15595

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
robmarkcole wants to merge6 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromrobmarkcole:add_in_autoscale

Conversation

robmarkcole
Copy link

PR Summary

TheArtist class gained anset_in_autoscale() method which
is used to determine if the instance is used in the autoscale calculation.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 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

@robmarkcolerobmarkcole changed the titleAdd in autoscaleAdd set_in_autoscale() methodNov 2, 2019
@robmarkcole
Copy link
Author

PR suggested by@anntzer

@anntzer
Copy link
Contributor

Note that this would not only be useful in itself, but would also fix a long-standing issue hinted at in

# TODO: the fix for the collections relim problem is to move the
# limits calculation into the artist itself, including the property of
# whether or not the artist should affect the limits. Then there will
# be no distinction between axes.add_line, axes.add_patch, etc.
# Collections are deliberately not supported (yet); see
# the TODO note in artists.py.

which basically asks for Collections to have such a flag -- and then we can get rid of the autolim kwarg in
defadd_collection(self,collection,autolim=True):

which is basically a one-off version of the property.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Generally, 👍

  1. How does this relate toautolim parameter ofadd_collection()?
  2. This conceptually is similar to the visibility checks inrelim().
    ifnotvisible_onlyorline.get_visible():

    It would be good to have both checks either inside the_update_*_limits or inrelim() but not distributed over two places.

@anntzer
Copy link
Contributor

  1. How does this relate to autolim parameter of add_collection()?

Places which used to calladd_collection(..., autolim=False) should just set the collection's in_autoscale property to False before passing it to add_collection (and add_collection should be updated to take it that property into account).

  1. This conceptually is similar to the visibility checks in relim(). It would be good to have both checks either inside theupdate*_limits or in relim() but not distributed over two places.

That's a bit tricky to do, as relim() is a public API but we should not modify the state of the artist depending on whether visible_only was passed to relim.

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

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

An example page would be nice..

@anntzer
Copy link
Contributor

will work on it

@anntzer
Copy link
Contributor

(Note to self: once this is merged this can also be used to control autolimits behavior in axline() as mentioned in the side note of#15330 (comment)).

@mwaskom
Copy link

It was mentioned in#15967 that this method (which solves exactly a problem that I having been having) applies to both axes in a plot, but there may be times when you want to exclude autoscaling on one axis but allow it on another. In fact, there is some precedence for that independence, e.g. with thescalex/scaley parameters inplt.plot.

timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
Member

I see the point of being able to specify this per axis.

Not completely thought this through API-wise, but aaxis parameter similar to the one intick_params might be the way to go.

@tacaswell
Copy link
Member

The input side of the API in clear and can be backwards ly, but what does theget side return? Do we want to do{'x': True, 'y': False}?

@mwaskom
Copy link

Ymmv but I think an (x, y) pair would be fine

@tacaswell
Copy link
Member

Blind tuples are pragmatic, but can lead to confusion if we start to lose track of what they mean.

On the other hand, that ambiguity can be leverage to play nicely with polar plots.

If we go with a tuple / dict / whatever we should also extend it to 3D as well?

@timhoffm
Copy link
Member

How about using a namedtuple?

@tacaswell
Copy link
Member

On a bit more consideration, I think that a tuple is probably the best path. The artists don't actually know if they are in an (x, y), (r, \theta), (ra, dec), or (lat, lon) coordinate system, they just know they are in 2D (or 3D) so trying to same them will end up causing more issues because it will be wrong rather than ambiguous.

timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
Member

timhoffm commentedNov 18, 2020
edited
Loading

API proposal:

Setter

set_in_autoscale(False)  # for both axisesset_in_autoscale((False, True)) # for each axis sset_in_autoscale((False, True, False)) # for 3D

Rationale: I find it more clear to have a single parameter that can be a scalar or a tuple compared to having variable number of arguments. In the latter case, we'd have toset_in_autoscale(*args) because you cannot have explicit arguments (set_in_autoscale(scalex, scaley)) and supporting a single value applying to all dimensions. But the single-parameter case is the primary usecase and excluding single dimensions is far more rare.

Getter

scalex, scaley = get_in_autoscale()scalex, scaley, scalez = get_in_autoscale()

Per#15595 (comment) a simple tuple is good enough and saves the hassle of determining meaningful names, which can depend on the projection.

Internal representation

self._in_autoscale should always be a tuple with as many entries as dimensions in the plot (2D/3D). The expansion from scalar to tuple should be done in the setter.

@robmarkcole Sorry this fell under the table. Are you still interested in working on this?

jklymak reacted with thumbs up emoji

@tacaswell
Copy link
Member

Rebased to use this as the start of the implementation for#25139

alexiscoutinho reacted with hooray emoji

@tacaswell
Copy link
Member

Tasks:

  • adopt tuple rather than a single bool
  • move the autolim logic into the Artist classes.

@robmarkcole Do you mind if we work in this PR / on your branch? I would rather do so because it keeps the discussion in one place, but also completely understand if you no longer are interested in getting notified about this discussion!

robmarkcole reacted with thumbs up emoji

@juseg
Copy link

This new property will be super useful! Could you explain the rationale for naming itin_autoscale instead of partly backward-compatibleautolim fromadd_collections? I am implemetingautolim in geopandas and wondering if I should call itin_autoscale instead. Thanks!

@fburgaud
Copy link

Any updates on this PR? would be a very useful feature.

bmcfee and edbennett reacted with thumbs up emoji

@timhoffm
Copy link
Member

timhoffm commentedFeb 18, 2025
edited
Loading

The following diagramm shows an outline of the limits-related call graph

limits-2025-02-18-210859

Currently the update is mostly done through theAxes methodsadd_[Artist] -->_update_[Artist]_limits -->update_datalim. Though there are quite a number of special cases:

  • _update_line_limits does not go through_update_datalim, but directly modifiesdataLim.
  • there is no_update_* method for Collections, insteadadd_collection directly callsupdate_datalim. Additionally, that is optional becauseadd_collection has a kwargautolim to decide whether data limits should be updated. Further speciality: Collections have aget_datalim method whereas in the other cases, the_update_[Artist] method must exctract the limits from the corresponding Artist.
  • Images are special in thatadd_image does not do autoscaling, butimshow callsAxesImage.set_extent, which in turn callsupdate_datalim. There is also a_update_image_limits method, but this is not called when adding the image, only when doing arelim().

So overall every Artist is a special case 😲.

I believe we should consolidate the architecture before modifying the autoscale behavior. We can do this in multiple steps:

  1. Investigate to remove the special cases: Canupdate_line_limits just callupdate_datalim? Canadd_image call_update_image_limits?
  2. I like theget_datalim method, as it's the responsibility of every Artist to know it's size in data coordinates. Every Artist that works with data should have this method, but it may return None. T.b.d. do we want to add a default impelmentation to Artist (which means also Figure grows this)? If not either we'd have to create the notion of DataArtist (either as base class or as protocol). I'm tempted to go with protocol because that is the least invasive.
  3. Remove the_update_[Artist]_limits methods.add_[Artist] andrelim() should callupdate_datalim(artist.get_datalim()).

To provide natural "in_autoscale" behavior, we'd need to take changes thoughArtist.set_in_autoscale into account. There are two ways: (1) Simple but expensive:set_in_autoscale() triggers a completerelim(). (2) Deferred limits update. Rearchitect futher so that data limits are calculated lazily. Basically autoscale would go through the artists and collect the limits.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak left review comments

@timhoffmtimhoffmtimhoffm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
Status: Waiting for author
Milestone
future releases
Development

Successfully merging this pull request may close these issues.

Add scale(x)(y) parameter(s) to imshow as in plot
10 participants
@robmarkcole@anntzer@mwaskom@timhoffm@tacaswell@juseg@fburgaud@jklymak@QuLogic@story645

[8]ページ先頭

©2009-2025 Movatter.jp