Blocked set_clim() callbacks to prevent inconsistent state (#29522) #29590
Conversation
There was a problem hiding this comment.
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 us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
There was a problem hiding this comment.
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 the set_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.
|
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 in 5defe48 |
256882f to
295e5b7
Compare
295e5b7 to
bf5370d
Compare
lib/matplotlib/tests/test_colors.py
Outdated
| plt.close(fig) | ||
|
|
There was a problem hiding this comment.
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.
| # Initial callback count should be zero | |
| assert callback.call_count == 0 | |
| callback.assert_not_called() |
2cbfdb3 to
49a0ab2
Compare
… 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