Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Blocked set_clim() callbacks to prevent inconsistent state (#29522)#29590
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add a test for this?
This is still a band-aid over the underlying problem in my opinion, but probably worth adding still. This won't help people setting vmin/vmax manually outside of theset_clim()
call.
The issue is still with who should own the updates to the norm, the colorbar or the norm setting and does one take precedence over the other. My guess is that we need to try and push the colorbar's auto-expansion logic up into the initializer somehow rather than at draw time.
Uh oh!
There was an error while loading.Please reload this page.
prafulgulani commentedFeb 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Not sure if the test would meet the expectations because this is my first open-source contribution, can I give it a try? |
@prafulgulani, yes please do give it a shot and if you have any questions feel free to ask here. You want to make sure that only one "changed" signal gets sent when calling matplotlib/lib/matplotlib/tests/test_colors.py Lines 1608 to 1630 in5defe48
|
256882f
to295e5b7
CompareUh oh!
There was an error while loading.Please reload this page.
295e5b7
tobf5370d
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me, thanks for updating this!
Note to second reviewer, please squash merge.
lib/matplotlib/tests/test_colors.py Outdated
plt.close(fig) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not necessary; tests always close figures.
plt.close(fig) |
lib/matplotlib/tests/test_colors.py Outdated
# Initial callback count should be zero | ||
assert callback.call_count == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
# Initial callback count should be zero | |
assertcallback.call_count==0 | |
callback.assert_not_called() |
2cbfdb3
to49a0ab2
Compare07371db
intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
… inconsistent state (matplotlib#29522)
Thanks@prafulgulani and congratulations on your first contribution to Matploblib! 🎉 |
closes#29522
Previously when norm's limits were updated, self.changed() was called through the callbacks attached to the norm, these callbacks were fired immediately leading to inconsistent state and causing the bug.
This change blocks the immediate callbacks and emits a update signal after both the limits have been set.
PR checklist