Skip to content

fix minor grid overlapping #11762

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 10, 2018
Merged

fix minor grid overlapping #11762

merged 1 commit into from
Aug 10, 2018

Conversation

ImportanceOfBeingErnest
Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Jul 24, 2018 •
edited
Loading

PR Summary

Possible fix for #11758.

Here we run into the classic problem of

0.06 % 0.02 != 0

numerically. So in addition to the condition a % b > something we also can check if a % b is not actually close to b itself (~np.isclose(a%b, b)).

Previously, the following code

import matplotlib.pyplot as plt

plt.ylim(0,  0.11) 
plt.minorticks_on()
plt.grid(True, 'minor', 'y', linewidth=3)
plt.grid(True, 'major', color='k', linewidth=3)

plt.show()

would result in an overlapping grid line at y=0.06,

image

With this PR the output is as expected.

image

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@ImportanceOfBeingErnest ImportanceOfBeingErnest mentioned this pull request Jul 24, 2018
Closed
@ImportanceOfBeingErnest
Copy link
Member Author

ImportanceOfBeingErnest commented Jul 24, 2018 •
edited
Loading

Still work in progress. Seems there is an issue with very small numbers where the additional condition would eat up all gridlines. fixed.

afvincent
afvincent reviewed Jul 24, 2018
View reviewed changes
lib/matplotlib/ticker.py Outdated
locs = locs.compress(cond)
mod = np.abs((locs - t0) % majorstep)
cond1 = mod > minorstep / 10.0
cond2 = ~np.isclose(mod, np.ones_like(mod)*majorstep)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cond2 = ~np.isclose(mod, majorstep) does not work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You prob need to set atol=0 and just use rtol (maybe with a larger value than default).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Yep, atol=0 did it.

@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the minor-grid-overlap branch 2 times, most recently from 879785b to 818c973 Compare July 25, 2018 00:43
@ImportanceOfBeingErnest
Copy link
Member Author

There is some problem with testing this.

The currently failing test only fails for the py3.5 environment with pinned environment, not the others. So it seems the Locator gives different output depending on some dependency.

What's more strange is that I initially would have liked to include

lim = (1.20e-06, 1.42e-06),
ref = [1.20625e-06, 1.21250e-06, 1.21875e-06, 1.23125e-06, 1.23750e-06,
 1.24375e-06, 1.25625e-06, 1.26250e-06, 1.26875e-06, 1.28125e-06,
 1.28750e-06, 1.29375e-06, 1.30625e-06, 1.31250e-06, 1.31875e-06,
 1.33125e-06, 1.33750e-06, 1.34375e-06, 1.35625e-06, 1.36250e-06,
 1.36875e-06, 1.38125e-06, 1.38750e-06, 1.39375e-06, 1.40625e-06,
 1.41250e-06, 1.41875e-06],

for testing as well, but that failed locally when testing through py.test

(env) > py.test \lib\matplotlib\tests\test_ticker.py::TestAutoMinorLocator

but it passes when being run outside the testing suite,

(env) \lib\matplotlib\tests\ > python
import test_ticker
s = test_ticker.TestAutoMinorLocator()
s.test_advanced(...)

So apparently the figures created within the test suite differ from the ones created through normal python?

@jklymak
Copy link
Member

jklymak commented Jul 25, 2018

For sure the test images can be different because it uses its own styles.

Its strange that you are getting a failure just on 3.5. How different are the minor ticks?

@jklymak jklymak added this to the v3.1 milestone Jul 26, 2018
@ImportanceOfBeingErnest
Copy link
Member Author

I did not find the reason why the 3.5 environment behaves differently. The issue is that creating minor ticks in the range (0, 0.11e-12) produces a tick at the edge value 0.11e-12 in the 3.5 environement, but not in the 3.6 environment.
Strangely enough, for the (0, 0.11e-19) case, both environments create a tick at 0.11e-19.
So I would think that the 3.5 environment is actually correct and the 3.6 environment behaves strangely.

In any case, I changed the limits for the test such that this problem is circumvented; because after all, we do not want to test the number of ticks with this, but rather whether minor tick positions overlap with major ones. This is what the test does now.

@ImportanceOfBeingErnest
Copy link
Member Author

tag - "sion" + "ew"

afvincent
afvincent approved these changes Aug 9, 2018
View reviewed changes
Copy link
Contributor

@afvincent afvincent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a reasonable fix to the problem, based on the description of the issue under the hood and the new extensive test passing.

lib/matplotlib/tests/test_ticker.py

ax.minorticks_on()
ax.grid(True, 'minor', 'y', linewidth=1)
ax.grid(True, 'major', color='k', linewidth=1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't something like

ax.grid(which="both", axis="y")  # or ax.grid(True, which="both", axis="y")

be enough to replace the 2 calls to `ax.grid`` (as one does not need the bare eye checking step here)? But even if that is the case, not really worth blocking the PR for so little IMHO...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess for the test itself we do not actually need any grid; this is more for the case that something goes wrong to easily see what it is. The point is, the previously present test should actually have made this issue apparent for anyone visualizing it with a grid - because it already had one number too much in it. So maybe keeping the grid in might help people looking at this in the future?

@afvincent
Copy link
Contributor

I am not 100 % comfortable with the side-issues reported by @ImportanceOfBeingErnest about the number of ticks, but I kind of agree that this seems to be outside of the current PR scope.

@jklymak
Copy link
Member

jklymak commented Aug 10, 2018

I'll milestone for a backport as I dont' think there are any issues backporting this and it fixes a bug...

@jklymak jklymak modified the milestones: v3.1, v2.2.4 Aug 10, 2018
@jklymak jklymak merged commit 4d1d12c into matplotlib:master Aug 10, 2018
lumberbot-app bot pushed a commit that referenced this pull request Aug 10, 2018
@jklymak
Backport PR #11762: fix minor grid overlapping
1f2b089
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 10, 2018
Merged
dstansby added a commit that referenced this pull request Aug 12, 2018
@dstansby
Merge pull request #11841 from meeseeksmachine/auto-backport-of-pr-11…
29ce489 
…762-on-v2.2.x

Backport PR #11762 on branch v2.2.x
@anntzer anntzer mentioned this pull request Jan 29, 2019
Merged
6 tasks
@ImportanceOfBeingErnest ImportanceOfBeingErnest deleted the minor-grid-overlap branch February 17, 2019 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@afvincent afvincent afvincent approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.2.4
Development

Successfully merging this pull request may close these issues.
4 participants
@ImportanceOfBeingErnest @jklymak @afvincent @anntzer