Skip to content

Move major/minor tick overstrike logic to Axis. #13314

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

Merged
merged 5 commits into from
Feb 23, 2019
Merged

Move major/minor tick overstrike logic to Axis. #13314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97eceb1
Move major/minor tick overstrike logic to Axis.
anntzer Jan 29, 2019
07ac300
Update test_log_scales to use check_figures_equal.
anntzer Jan 29, 2019
7c201fd
Update test_outward_ticks to check for axes positions after tightlayout.
anntzer Jan 29, 2019
dbc693a
Remove png/svg versions of test_symlog{,2}.
anntzer Jan 29, 2019
a752d46
Clarify a bit iter_ticks.
anntzer Jan 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions doc/api/next_api_changes/2018-01-30-AL.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Minor Locator no longer try to avoid overstriking major Locators
````````````````````````````````````````````````````````````````

Previously, certain locator classes (`LogLocator`, `AutoMinorLocator`)
contained custom logic to avoid emitting tick locations that collided with
major ticks when they were used as minor locators.

This logic has now moved to the Axis class; thus, for example,
``xaxis.minor.locator()`` now includes positions that collide with
``xaxis.major.locator()``, but ``xaxis.get_minorticklocs()`` does not.
39 changes: 24 additions & 15 deletions lib/matplotlib/axis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,17 +1013,15 @@ def _set_artist_props(self, a):

def iter_ticks(self):
"""
Iterate through all of the major and minor ticks.
Yield ``(Tick, location, label)`` tuples for major and minor ticks.
"""
major_locs = self.major.locator()
major_ticks = self.get_major_ticks(len(major_locs))
major_locs = self.get_majorticklocs()
major_labels = self.major.formatter.format_ticks(major_locs)

minor_locs = self.minor.locator()
minor_ticks = self.get_minor_ticks(len(minor_locs))
minor_labels = self.minor.formatter.format_ticks(minor_locs)

major_ticks = self.get_major_ticks(len(major_locs))
yield from zip(major_ticks, major_locs, major_labels)
minor_locs = self.get_minorticklocs()
minor_labels = self.minor.formatter.format_ticks(minor_locs)
minor_ticks = self.get_minor_ticks(len(minor_locs))
yield from zip(minor_ticks, minor_locs, minor_labels)

def get_ticklabel_extents(self, renderer):
Expand Down Expand Up @@ -1304,18 +1302,29 @@ def get_ticklines(self, minor=False):
return self.get_majorticklines()

def get_majorticklocs(self):
"Get the major tick locations in data coordinates as a numpy array"
"""Get the array of major tick locations in data coordinates."""
return self.major.locator()

def get_minorticklocs(self):
"Get the minor tick locations in data coordinates as a numpy array"
return self.minor.locator()
"""Get the array of minor tick locations in data coordinates."""
# Remove minor ticks duplicating major ticks.
major_locs = self.major.locator()
Copy link
Member

Choose a reason for hiding this comment

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

why not pass the major_locs here, if we don't want to do it in the locator? Agree that its an API change, but I suspect a minor one; this method is not used elsewhere, except in the tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because if anything I think the main API change here is that one cannot call axis.minor.locator() to get the minor tick locations. The main API to do that is now axis.get_minorticklocs(); making it axis.get_minorticklocs(major_locs=axis.get_majorticklocs()) seems a bit of a pain (Also, what about axis.get_ticklocs(minor=True)? Would you now have to pass the major_locs kwarg to get_ticklocs() but only if minor=True? Looks a bit awkward.).

minor_locs = self.minor.locator()
Copy link
Member

Choose a reason for hiding this comment

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

aren't we now calling both locators twice? Seems inefficient to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes (per the above).
Frankly, would not worry as I doubt it's anywhere close to being a bottleneck, but we could factor out get_minorticklocs to a private helper that takes majorlocs as parameter if this is really an issue...

Copy link
Member

Choose a reason for hiding this comment

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

OK, actually I'm confused - why are we calling the locator at all here? Do we not cache the existing tick locations in some way? Sorry if I'm being dumb.

Copy link
Contributor Author

@anntzer anntzer Feb 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I... don't think so? Note that the previous implementation also called the locator.
Obviously we could inspect what are the current tick positions, but I don't know if there's any invalidation mechanism in place right now (e.g. what happens when you pan/zoom?).
IOW, this uses the same structure as before the PR; having get_{major,minor}ticklocs handle cache invalidation could be future work ( :) ).

transform = self._scale.get_transform()
tr_minor_locs = transform.transform(minor_locs)
tr_major_locs = transform.transform(major_locs)
lo, hi = sorted(transform.transform(self.get_view_interval()))
# Use the transformed view limits as scale. 1e-5 is the default rtol
# for np.isclose.
tol = (hi - lo) * 1e-5
Copy link
Member

Choose a reason for hiding this comment

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

"transformed view limits" means this is now in axes coordinates, right? If so, this is the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's some linear transform away from axes coordinates, so the relative check is equivalent to being done in axes coordinates.

minor_locs = [
loc for loc, tr_loc in zip(minor_locs, tr_minor_locs)
if not np.isclose(tr_loc, tr_major_locs, atol=tol, rtol=0).any()]
return minor_locs

def get_ticklocs(self, minor=False):
"Get the tick locations in data coordinates as a numpy array"
if minor:
return self.minor.locator()
return self.major.locator()
"""Get the array of tick locations in data coordinates."""
return self.get_minorticklocs() if minor else self.get_majorticklocs()

def get_ticks_direction(self, minor=False):
"""
Expand Down
Binary file removed lib/matplotlib/tests/baseline_images/test_axes/symlog.png
View file
Open in desktop
Binary file not shown.
Loading