CenteredNorm changes #24132
CenteredNorm changes #24132
Conversation
| # set vcenter to 1, which should double halfrange | ||
| # set vcenter to 1, which should move the center but leave the | ||
| # halfrange unchanged |
There was a problem hiding this comment.
This is what I found odd. Updating vcenter should double the halfrange? Those seem like the two fixed quantities to me and one shouldn't update the other. I think this would better be handled by doing an autoscale after setting vcenter if you want this functionality.
|
Based on today's call I agree with the changes here, though the backcompat issue will need to be decided on. |
1aeaf05 to
f259067
Compare
|
I added a "behavior change" release note identifying the change in behavior for that specific aspect of this update. |
|
|
||
| Previously, the halfrange would expand in proportion to the | ||
| amount that vcenter was moved away from either vmin or vmax. Now, | ||
| the halfrange will not be updated when vcenter is changed. If the |
There was a problem hiding this comment.
I'm maybe slightly confused about how vcenter, vmin, vmax, and half range stayed in sync before this change and stay in sync after this change and I think a sentence on each might make the change more clear?
There was a problem hiding this comment.
I agree, it is confusing what was happening vs whats new. I added a couple of code blocks to try and demonstrate what previously happened and what is happening now. It seems a bit long though still...
There was a problem hiding this comment.
I think it's fine, especially since this is the behavior change rather than the whats new and the behavior changes page tends to be not as packed w/ stuff.
| # norm.halfrange == 1, vmin == 0, vmax == 2 | ||
|
|
||
| The **halfrange** can be set manually or ``norm.autoscale()`` | ||
| can be used to autmatically set the limits after setting **vcenter**. |
There was a problem hiding this comment.
| can be used to autmatically set the limits after setting **vcenter**. | |
| can be used to automatically set the limits after setting **vcenter**. |
spelling
There was a problem hiding this comment.
👍 fixed, and squashed the commits.
This changes CenteredNorm to use vmin and vmax to represent the halfrange rather than storing it separately and needing to update the vmin/vmax in all of the methods. Additionally, if you now change vcenter, the halfrange does not automatically update.
94dc568 to
84def85
Compare
| def vmin(self, value): | ||
| value = _sanitize_extrema(value) | ||
| if value != self._vmin: | ||
| self._vmin = value | ||
| self._vmax = 2*self.vcenter - value | ||
| self._changed() | ||
|
|
There was a problem hiding this comment.
I have a slight confusion - what happens to half range when these are changed?
There was a problem hiding this comment.
halfrange is an @property which is dynamically computed from vmin/vmax and is not held in memory directly (and inversely sets vmin/vmax in the @halfrange.setter)
There was a problem hiding this comment.
halfrange is defined by vmin/vmax now, so the halfrange is shrunk/expanded accordingly. I added a couple of tests at the end to demonstrate what the expected behavior of this is. (vcenter constant, halfrange changed)
There was a problem hiding this comment.
thanks I missed that part of the code 😅
There was a problem hiding this comment.
ok, so just to confirm, basically calling self.halfrange(val) updates the min and max, so that then self.halfrange = self.halfrange() which is why there's no self._halfrange - sorry I'm tired
There was a problem hiding this comment.
yes
PR Summary
This updates
CenteredNormto be consistent between all setting/getting ofvcenter,halfrange,vmin, andvmax. The norm is defined byvcenterandhalfrange, andvmin/vmaxare adjusted accordingly.Currently, if we change the center of the norm, the halfrange also gets updated based on the distance away from the previous
vmin/vmaxvalues. This "feature" is hard to maintain in this refactor, so I'm going to propose that this should be changed to not modifyhalfrange(i.e. if I updatevcenterI just pan the center limit and keep the linear scaling out from the center fixed). I'm not sure how easy it would be to deprecate, so I think this needs some discussion and should not be backported to 3.6.Additionally, now setting
vminorvmaxis equivalent to updating thehalfrangefrom eithervcenter - vminorvmax - vcenter.xref: #24093
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).