FIX: Guard against already-removed labels in ContourSet.remove()#31431
FIX: Guard against already-removed labels in ContourSet.remove()#31431ksunden merged 9 commits intomatplotlib:mainfrom
Conversation
rcomer
left a comment
There was a problem hiding this comment.
Thanks @buddy0452004, but I do not think you understood the root cause of why the failure happens. I think the explanation at #31404 (comment) is correct. The error shown in the issue states that we tried to remove something from a list that is not in the list. So we got hold of the list axes.texts without problem, but the labels were no longer in there.
lib/matplotlib/tests/test_contour.py
Outdated
|
|
||
| def test_contour_remove_after_label_removed(): | ||
| # Test that CS.remove() works even if labels were manually removed first | ||
| # Regression test for https://github.com/matplotlib/matplotlib/issues/31404 |
There was a problem hiding this comment.
We do not typically reference issues in test comments.
lib/matplotlib/tests/test_contour.py
Outdated
| x = np.linspace(-3.0, 3.0, 20) | ||
| y = np.linspace(-2.0, 2.0, 20) | ||
| X, Y = np.meshgrid(x, y) | ||
| Z = np.exp(-X**2 - Y**2) |
There was a problem hiding this comment.
For efficiency, try to use the simplest data input you can that still makes the test effective. Look at test_contourf_rasterize for an example.
lib/matplotlib/tests/test_contour.py
Outdated
|
|
||
|
|
||
|
|
||
| def test_contour_remove_after_label_removed(): |
There was a problem hiding this comment.
Since there is already a test_contour_remove further up the file, I would put this one right below it, to keep tests with similar purposes together.
lib/matplotlib/contour.py
Outdated
| for text in self.labelTexts: | ||
| text.remove() | ||
| for text in list(self.labelTexts): | ||
| if axes is not None and text in axes.texts: |
There was a problem hiding this comment.
Why do we check whether axes is None here?
|
@rcomer Thanks for the review! You're right about the root cause the issue is that the labels were already removed from Also:
|
|
I'm putting this on hold until we have clarified what is actually needed. #31404 (comment) |
|
@timhoffm @rcomer Updated based on the discussion on #31404:
|
According to me, the minimal approach related to this issue by adding the note in clabel() is enough as these changes affecting the colorbar ticks . |
Sorry, I'm not following. What is happening to the colorbar ticks? |
But I am not 100% sure that, this error is related to code or not . |
|
That one is a sporadic flaky test, so not related to this PR. |
|
I had removed all labels in a loop thinking it would better trigger the warning, and added a second cs.clabel() call thinking it would make the scenario more complete but neither was relevant to what the test is actually checking. Simplified to just removing one label as suggested. |
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
The CircleCI docs failure is just a network hiccup on the CI side. The build log shows a 503 Service Unavailable error when it tried to fetch the Python docs inventory (docs.python.org/3/objects.inv) so all the intersphinx references like warnings.warn, functools.partial etc couldn't resolve and that generated 321 warnings which the docs build treats as errors. Nothing in this PR caused it. Can someone retrigger the CI or should I close and reopen? |
|
The doc build now passes. |
When contour labels are manually removed before calling CS.remove(), matplotlib crashes with a ValueError.
Based on discussion in #31404, this PR:
Closes #31404
AI Disclosure
No AI used.
PR Checklist