FIX: Guard against already-removed labels in ContourSet.remove()#31431
FIX: Guard against already-removed labels in ContourSet.remove()#31431buddy0452004 wants to merge 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 <[email protected]>
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