Fixed Issue 4346 #6079
Fixed Issue 4346 #6079
Conversation
_apply_params was calling setattr on _size and _color instead of _labelsize and _labelcolor
|
Can you also add a test? I would suggest a non-image test where you exercise this and assert that the tick size / color does not change due to it. Ideally the tests should only use public methods. Also 👍 on tracking this one down, this looks subtle as all get out. |
|
I will work on this |
test will make two calls two set_tick_params. After the fix the second call should not change any of the ticks color or size values.
The second call to set_tick_params would mess the padding up only after the first call was causing size and color to be set incorrectly. Thus the test is changed to ensure the root cause is fixed and not the propagation of it
|
Hopefully this is sufficient. |
| axis_1.yaxis.set_tick_params(labelsize=30, labelcolor='red', direction='out') | ||
|
|
||
| # Expected values after setting the ticks | ||
| assert axis_1.yaxis.majorTicks[0]._size == 4.0 |
There was a problem hiding this comment.
Is there any reliably way to test this using public API?
There was a problem hiding this comment.
The reason for asking that is that, long term, we reserve the right to change the private API with no warning or deprecation cycle. Tests that only use public API give confidence that we have done that right but that is eroded if we have to touch the tests as part of the refactor as well.
There was a problem hiding this comment.
That makes sense, I didn't consider the public API. I will look into this.
There was a problem hiding this comment.
I can't seem to find a way to get either of labelsize, labelcolor, size, or color out of the public API. There is a get_markersize in the docs that would return _size, however I don't see it in the code. Should I implement this?
There was a problem hiding this comment.
If there is not a good public API, then this is fine. Creating new API for the purpose of testing it is not a good idea!
|
backported to 1.5.x as b4c3091 |
|
Thanks! And congratulations in what I think is your first mpl contribution 👏 In the future, it is best to make PRs off of a feature branch, not your master branch. |
|
Will do, and thanks! |
This is a fix for issue #4346
The problem was that when handling parameters labelsize and labelcolor, _apply_params was calling setattr on _size and _color instead of _labelsize and _labelcolor. This is all fine and dandy until something is used that calls _apply_params because _apply_params calls apply_tickdir which will change the padding.
When _size changes, its value gets added to the tick padding thus causing the issue seen in #4346. This also explains why the issue only became apparent when setting major (or both) ticks, it is because minor ticks don't have _size added to their padding values.