| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Thanks for opening this PR! Could you give a bit more context to what future work this change will enable? |
Sorry, something went wrong.
There was a problem hiding this comment.
This is not yet appropriate
To be clear, this PR must provide the full in autoscale logic, shifting the in-behavior hard-coded logic to only depending on the artist property.
Sorry, something went wrong.
…elim regression test (to be added in follow-up PR)
|
The scatter relim regression test was removed from this PR since it exercises follow-up behavior (PR2). I’ll reintroduce it once PathCollection autoscaling is wired via the new artist flag. |
Sorry, something went wrong.
|
Thanks for the clarification — I’ve updated the PR so autoscale participation is fully artist-driven. The flag is now initialized in the add_* methods to preserve existing behavior, and relim() only gates on the artist property. |
Sorry, something went wrong.
|
@dstansby Thanks for the question! The goal of this change is to make inclusion in autoscaling an explicit, artist-level property, rather than relying on hard-coded type checks spread across Axes methods. In the short term, this PR does not change behavior; it only introduces the minimal infrastructure needed to express whether an artist participates in autoscaling in a uniform way. This enables follow-up work to: -Make add_collection(..., autolim=...) and similar APIs consistent across artist types. -Decouple autoscaling decisions from specific artist classes, allowing more predictable and testable behavior. -Incrementally refactor limit handling to query artists for their autoscale participation, instead of embedding logic in Axes.relim and related methods. -Concretely, this is intended to support the follow-up PRs discussed above, where conditional autoscaling of collections and a more unified limits architecture can be implemented without further ad-hoc checks. |
Sorry, something went wrong.
| if a.get_clip_path() is None: | ||
| a.set_clip_path(self.patch) | ||
| self.stale = True | ||
| a._set_in_autoscale(False) |
There was a problem hiding this comment.
is this needed?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes — the intent was to make the current behavior explicit.
add_artist historically does not opt the artist into autoscaling, so setting _in_autoscale(False) there makes that behavior explicit and avoids relying on defaults. This keeps relim() fully driven by the artist property rather than by implicit assumptions.
Sorry, something went wrong.
There was a problem hiding this comment.
I'd argue otherwise: For all standard cases this does nothing, because the default value is False. It would only have an effect a user would manually do artist.set_in_autoscale(True); ax.add_artist(artist). But we just have created set_in_autoscale() so that we don't have do consider backward compatibility in that case. And logically, overriding the in_autoscale settting through add_artist() would be surprising. So it's better to leave this out here and rely on the global default that an artist does not take part in autoscaling, unless we've explicitly opted in for that.
Sorry, something went wrong.
| ---------- | ||
| b : bool | ||
| """ | ||
| self._in_autoscale = b |
There was a problem hiding this comment.
To be checked: Do we need to set the self._stale flag?
Sorry, something went wrong.
There was a problem hiding this comment.
I don’t think _set_in_autoscale should mark the artist as stale.
The flag only affects whether the artist participates in future autoscaling; it doesn’t change the artist’s visual state or require a redraw on its own. In practice it’s set during add_*, before any drawing occurs, so marking it stale would be unnecessary.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, so the argument is that changing in_autoscale will not result in automatic recalculation of the limits. Accepted.
Can you please add a sentence on this to the docstring.
Sorry, something went wrong.
| self._request_autoscale_view() | ||
|
|
||
| self.stale = True | ||
| collection._set_in_autoscale(autolim) |
There was a problem hiding this comment.
For the scope of the PR, this does not have any effect as collections do not take part in limit calculation. So we could leave this out.
Note that currently the autolim parameter is a one-time action to update the data limits.
You may keep it in anticipation of the upcoming changes for #31128. But if you do, you should move it to before if autolim: and add a comment that this does not have any effect yet.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the clarification — that makes sense.
I’ll remove this for now and keep the PR focused on introducing the artist-level flag without anticipating collection behavior. We can wire this up properly once collections participate in limit calculation.
Sorry, something went wrong.
| def _update_collection_limits(self, collection): | ||
| offsets = collection.get_offsets() | ||
| if offsets is not None and len(offsets): | ||
| self.update_datalim(offsets) | ||
|
|
||
|
|
There was a problem hiding this comment.
Please remove. This is not within the scope of this PR but belongs in #31128.
Sorry, something went wrong.
There was a problem hiding this comment.
Sure — removed. I’ll keep collection limit handling for #31128 and keep this PR focused on introducing the artist-level autoscale flag only.
Sorry, something went wrong.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
Thanks for the review and guidance! |
Sorry, something went wrong.
|
I suppose this should have been squash-merged, or at least rebased without extraneous changes, as #31128 has now been marked as merged even though all of those changes were reverted in follow-up commits in this PR, so #31128 is effectively not merged. |
Sorry, something went wrong.
|
Yes, this should have been squashed-merged. :/ |
Sorry, something went wrong.
|
Thanks for the review and guidance throughout this PR. |
Sorry, something went wrong.
|
I'm planning to apply for GSoC this year and would love to continue contributing to autoscaling improvements. Are there related issues where help would be valuable? |
Sorry, something went wrong.
|
Eventually, we want a public way to mark whether artists should take part in autoscaling #15595. Note also, that it could be worth to make this a per-axis setting, so that th artist takes part in x-autoscaling but not in y. As a prerequisite we should standardize and simplify the current mess of limits handling #15595 (comment) |
Sorry, something went wrong.
…autoscale-flag
…autoscale-flag
| Back | FazBrowse Home | New Git URL |
Introduce a private Artist-level _in_autoscale flag (mirroring _in_layout) and gate autoscaling centrally in Axes.relim().
This PR is preparatory infrastructure only and does not intend to change existing behavior. Follow-up work will build on this to address collection.