FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Move major/minor tick overstrike logic to Axis. by anntzer · Pull Request #13314 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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
jklymak merged 5 commits into from
Feb 23, 2019
Conversation 36 Commits 5 Checks 0 Files changed
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 .pdf  (2) .png  (4) .py  (5) .rst  (1) .svg  (4) All 5 file types selected
Deleted files Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
10 changes: 10 additions & 0 deletions doc/api/next_api_changes/2018-01-30-AL.rst
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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)
Comment thread
anntzer marked this conversation as resolved.
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()

jklymak Jan 30, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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...

anntzer Jan 30, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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()

jklymak Jan 30, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

anntzer Jan 30, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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...

jklymak Feb 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

anntzer Feb 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

timhoffm Feb 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

anntzer Feb 16, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Show comments
View file
Open in desktop
Binary file not shown.
Loading
Back | FazBrowse Home | New Git URL