| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…) previously skipped Collection instances entirely. Two issues: 1. add_collection() did not call collection._set_in_autoscale(True), unlike add_line(), add_patch(), and add_image(). This caused relim() to skip collections via the _get_in_autoscale() check. 2. relim() had no handling for Collection instances. Added an elif branch that calls get_datalim() and update_datalim(), mirroring the logic already used in add_collection(). Closes matplotlib#30859
|
Please post a screenshot showing the result of the script from the issue. Please also fill out the PR summary using our template. |
Sorry, something went wrong.
|
⏰ This pull request might be automatically closed in two weeks from now. Thank you for your contribution to Matplotlib and for the effort you have put into this PR. This pull request does not yet meet the quality and clarity standards needed for an effective review. Project maintainers have limited time for code reviews, and our goal is to prioritize well-prepared contributions to keep Matplotlib maintainable. Matplotlib maintainers cannot provide one-to-one guidance on this PR. However, if you ask focused, well-researched questions, a community member may be willing to help. 💬 To increase the chance of a productive review:
As the author, you are responsible for driving this PR, which entails doing necessary background research as well as presenting its context and your thought process. If you are a new contributor, or do not know how to fulfill these requirements, we recommend that you familiarize yourself with Matplotlib's development conventions or engage with the community via our Discourse or one of our meetings before submitting code. If you substantially improve this PR within two weeks, leave a comment and a team member may remove the status: autoclose candidate label and the PR stays open. Cosmetic changes or incomplete fixes will not be sufficient. Maintainers will assess improvements on their own schedule. Please do not ping (@) maintainers. |
Sorry, something went wrong.
Mirror the comments from add_collection() that explain why minpos is included (log scale support) and why contains_branch_separately is used to conditionally update x/y limits.
|
Hi, just wanted to flag that this PR has been significantly updated since it was first opened. The description now includes a full root-cause explanation, a minimal reproducer with before/after images, a dedicated test (test_relim_collection), and a behavior change note in doc/api/next_api_changes/. Happy to answer any questions or make adjustments based on feedback — thanks for taking a look! |
Sorry, something went wrong.
_set_in_autoscale(True) was set unconditionally in add_collection(), outside the 'if autolim:' block. This meant that any collection added with autolim=False would still be picked up by relim() later. Fix: move _set_in_autoscale(True) inside the 'if autolim:' block so that relim() only considers collections that explicitly opted in. Add two regression tests: - test_relim_collection_autolim_false: verifies that a collection added with autolim=False does not affect limits after relim(). - test_relim_collection_log_scale: verifies that relim() + autoscale_view() works correctly for a Collection on log-scaled axes (exercises the minpos path). Closes matplotlib#30859
|
Pushed one more focused update to address a semantic issue in the fix. _set_in_autoscale(True) was being called unconditionally in add_collection(), outside the if autolim: block — meaning collections added with autolim=False would silently participate in relim() anyway, breaking the opt-out contract. Changes in this update:
The goal remains a narrow bugfix for #30859, not a redesign of autoscale semantics. |
Sorry, something went wrong.
| elif isinstance(artist, mimage.AxesImage): | ||
| self._update_image_limits(artist) | ||
| elif isinstance(artist, mcoll.Collection): | ||
| datalim = artist.get_datalim(self.transData) |
There was a problem hiding this comment.
This is on the wrong abstraction level and duplicates code from add_collection
If you want a narrow fix, please add _update_collection_limits and call it here - even if #15595 (comment) suggests that the update functions should eventually be removed.
Sorry, something went wrong.
There was a problem hiding this comment.
Done — extracted _update_collection_limits() following the same pattern as _update_line_limits, _update_patch_limits, and _update_image_limits. Both add_collection() and relim() now call the helper instead of repeating the get_datalim / minpos-concatenation / contains_branch_separately / update_datalim block inline. See commit 68a6269.
Sorry, something went wrong.
…on and relim() Refactor per reviewer feedback (timhoffm): the Collection data-limit logic in relim() was duplicating code from add_collection(). Extract it into a new private method _update_collection_limits(), following the existing pattern of _update_line_limits(), _update_patch_limits(), and _update_image_limits(). Both add_collection() and relim() now call the helper instead of repeating the get_datalim / minpos-concatenation / contains_branch_separately / update_datalim block inline.
There was a problem hiding this comment.
On a closer look, I'm unclear whether it is reasonable to have the "small" fix of relim() using
Sorry, something went wrong.
| ax.autoscale_view() | ||
| xlim = ax.get_xlim() | ||
| ylim = ax.get_ylim() | ||
| assert xlim[0] <= 1 and xlim[1] >= 3 | ||
| assert ylim[0] <= 4 and ylim[1] >= 6 |
There was a problem hiding this comment.
relim works on ax.dataLim. Please test its exact values and not the view limits.
Sorry, something went wrong.
There was a problem hiding this comment.
| Since 3.11 `autolim=True` matches the standard behavior | |
| of other ``add_[artist]`` methods: Axes data and view limits | |
| are both updated in the method, and the collection will | |
| be considered in future data limit updates through | |
| `.relim`. | |
| Prior to matplotlib 3.11 this was only a one-time update | |
| of the data limits. Updating view limits required an | |
| explicit calls to `~.Axes.autoscale_view`, and collections | |
| did not take part in `.relim`. |
Sorry, something went wrong.
| # Mark collection as participating in relim() only when autolim | ||
| # is enabled. If autolim=False the caller explicitly opted out, | ||
| # so relim() must not pick this collection up later. | ||
| collection._set_in_autoscale(True) |
There was a problem hiding this comment.
Should we revert the logic here? Historically, Collections did not take part in autoscaling. When introducing the in_autoscale flag, we therefore defaulted it to False on Collections, but True for all other Artists.
For better consistency we may now want Collections defaulting to True as well and explicitly set to False in case of autolim=False as special-cased backward-compatibility. Am I missing a point why we can't do that?
Sorry, something went wrong.
|
Addressed the latest review concerns with a narrow-scope update focused on #30859:
I intentionally kept this PR narrow (bugfix + behavior/docs/tests alignment) rather than taking the broader autoscale architecture route, which seems better handled in follow-up design work. |
Sorry, something went wrong.
Line data [0, 1] x [0, 1] gives minpos=[1., 1.] (minimum *positive* value), not [inf, inf]. The [inf, inf] sentinel only appears when there are no positive values at all.
| # Keep relim() participation aligned with the autolim argument. | ||
| # autolim can also be the internal sentinel "_datalim_only". | ||
| collection._set_in_autoscale(bool(autolim)) |
There was a problem hiding this comment.
Sorry, my bad I had a wrong recollection of Artist._in_autoscale being True by default. It's actually False, and set to True in the add_[artist] methods. So this should move back into if autolim in form of a simple _set_in_autoscale(True)
Sorry, something went wrong.
| The collection to add. | ||
| autolim : bool | ||
| Whether to update data and view limits. | ||
| Whether to update data limits and request autoscaling. |
There was a problem hiding this comment.
The original wording was clearer to the user. "request autoscaling" is an internal concept for lazy evaluation of autoscaled view limits. The user should not see a difference between "request autoscaling" and an immediate update of the view limits. Please revert.
Sorry, something went wrong.
| If *False*, the collection is explicitly excluded from | ||
| `~.Axes.relim`. |
There was a problem hiding this comment.
| If *False*, the collection is explicitly excluded from | |
| `~.Axes.relim`. | |
| If *False*, the collection does not take part in any limit | |
| operations. |
Sorry, something went wrong.
…toscale semantics
- Revert autolim docstring first line to original user-facing wording
("update data and view limits" instead of internal "request autoscaling")
- Adopt reviewer suggestion for If-False clause: "does not take part in
any limit operations"
- Expand versionchanged:: 3.11 note to explain both pre-3.11 behavior
and the new relim participation (per reviewer suggestion)
- Move _set_in_autoscale(True) inside if autolim: block (simpler and
consistent with how add_line/add_patch/add_image do it)
- Update stale TODO in Artist.remove: the "collections relim problem"
referenced has been fixed; rephrase to reflect the remaining
long-term architectural goal
|
Addressed the latest reviewer comments in commit a1c58ec:
All three regression tests pass locally (test_relim_collection, test_relim_collection_autolim_false, test_relim_collection_log_scale). |
Sorry, something went wrong.
_set_in_autoscale(True) must not be called for collections added with
autolim='_datalim_only' — that sentinel means 'update datalim once but
do not enter the autoscale/relim system'. Previously, because the call
was outside the if autolim: block, bool('_datalim_only') == True meant
3D collections inadvertently got _in_autoscale=True. Fixed by gating
the call on autolim != '_datalim_only', consistent with the existing
_request_autoscale_view guard directly below.
Also add test_relim_collection_autoscale_view: an end-to-end regression
that checks ax.get_xlim()/get_ylim() after relim()+autoscale_view(),
matching the exact user-facing scenario from GH#30859.
|
One more focused update (f90c147) fixing a subtle semantic issue I caught during review: _datalim_only semantics fix — _set_in_autoscale(True) was being called for collections added with autolim="_datalim_only" (the internal 3D sentinel), because the truthy string would pass the if autolim: check. That sentinel means "update datalim once, do not enter the autoscale/relim system", so these collections should keep _in_autoscale=False. Fixed by gating the call on autolim != "_datalim_only", consistent with the existing _request_autoscale_view guard directly below it. 141 non-image 3D tests pass. New end-to-end test — added test_relim_collection_autoscale_view: reproduces the exact user-facing scenario from #30859 by checking ax.get_xlim()/get_ylim() after relim() + autoscale_view(), not just dataLim. All 5 relim tests pass locally. |
Sorry, something went wrong.
- Fix backticks on autolim=True in add_collection docstring (single backtick was interpreted as cross-reference by Sphinx) - 'explicit calls' -> 'explicit call' per reviewer suggestion - Remove out-of-place TODO comments from artist.py remove()
|
Thanks @tinezivic, and congratulations on your first contribution to matplotlib! 🎉 We hope to see you back. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR summary
relim() completely ignored Collection artists (scatter plots, PathCollection, etc.) — calling ax.relim() after scatter.set_offsets() had no effect on the axis limits. Closes #30859.
Why is this change necessary?
Users who update scatter data dynamically (e.g. animation loops, interactive plots) rely on relim() + autoscale_view() to recalculate axis limits. For line artists this works; for scatter/Collection artists it silently did nothing.
What problem does it solve?
After scatter.set_offsets(new_data), calling ax.relim(); ax.autoscale_view() now correctly updates the axis limits to fit the new data.
Root cause — two bugs, one fix:
add_collection() never called collection._set_in_autoscale(), unlike add_line(), add_patch(), and add_image(). As a result relim() skipped all Collections because _get_in_autoscale() returned False (the default). The fix calls _set_in_autoscale(bool(autolim)) so that autolim=False correctly opts the collection out of future relim() calls.
relim() in _base.py had no elif isinstance(a, mcoll.Collection) branch. Even with flag fixed, the data limits would never have been picked up. The fix adds that branch, calling a new _update_collection_limits() helper that mirrors the logic already used in add_collection().
Minimal reproduction:
Before fix — axis stays at default [0, 1], data is cut off:
After fix — axis correctly scales to data extent:
AI Disclosure
I used GitHub Copilot (Claude Sonnet 4.6) to assist with code generation, diff review, and drafting documentation (including doc/api/next_api_changes/behavior/31530-TZ.rst). The root cause analysis, understanding of the _in_autoscale mechanism, identification of both bug sites, and verification of the fix logic were done by me.
PR checklist