/matplotlib

Fixed Issue 4346 #6079

Merged
merged 3 commits into from Mar 4, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  v1.5.2
3 participants
@paulkirow @tacaswell @mdboom
@paulkirow
Contributor

paulkirow commented Feb 29, 2016

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.
@paulkirow paulkirow Fixed issue 4346 
_apply_params was calling setattr on _size and _color instead of _labelsize and _labelcolor

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
a202c2c

mdboom added the needs_review label Feb 29, 2016

@tacaswell
Owner

tacaswell commented Mar 1, 2016

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.

tacaswell added this to the 1.5.2 (Critical bug fix release) milestone Mar 1, 2016

@paulkirow
Contributor

paulkirow commented Mar 1, 2016

I will work on this

paulkirow added some commits Mar 1, 2016

@paulkirow paulkirow Created test for previous issue 
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.

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
58572ce
@paulkirow paulkirow Removed redundancies in test 
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

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
d103d48
@paulkirow
Contributor

paulkirow commented Mar 1, 2016

Hopefully this is sufficient.

@tacaswell tacaswell commented on the diff Mar 2, 2016

View full changes
lib/matplotlib/tests/test_axes.py
@@ -4335,6 +4335,17 @@ def test_pandas_indexing_hist():
fig, axes = plt.subplots()
axes.hist(ser_2)
+@cleanup
+def test_axis_set_tick_params_labelsize_labelcolor():
+ # Tests fix for issue 4346
+ axis_1 = plt.subplot()
+ 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
@tacaswell

tacaswell Mar 2, 2016

Owner

Is there any reliably way to test this using public API?

@tacaswell

tacaswell Mar 2, 2016

Owner

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.

@paulkirow

paulkirow Mar 2, 2016

Contributor

That makes sense, I didn't consider the public API. I will look into this.

@paulkirow

paulkirow Mar 2, 2016

Contributor

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?

@tacaswell

tacaswell Mar 2, 2016

Owner

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!

@tacaswell tacaswell added a commit that referenced this pull request Mar 4, 2016

@tacaswell tacaswell Merge pull request #6079 from paulkirow/master 
Fix: Issue #4346
 a44571a

@tacaswell tacaswell merged commit a44571a into matplotlib:master Mar 4, 2016
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

tacaswell removed the needs_review label Mar 4, 2016

@tacaswell tacaswell added a commit that referenced this pull request Mar 4, 2016

@tacaswell tacaswell Merge pull request #6079 from paulkirow/master 
Fix: Issue #4346
 b4c3091
@tacaswell
Owner

tacaswell commented Mar 4, 2016

backported to 1.5.x as b4c3091
@tacaswell
Owner

tacaswell commented Mar 4, 2016

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.
@paulkirow
Contributor

paulkirow commented Mar 4, 2016

Will do, and thanks!

@tacaswell tacaswell added a commit to tacaswell/matplotlib that referenced this pull request May 22, 2016

@tacaswell tacaswell Merge pull request #6079 from paulkirow/master 
Fix: Issue #4346
 6f52ee0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment