Skip to content
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

Enh optional tick deconflict #13826

Closed
wants to merge 3 commits into from
Closed

Enh optional tick deconflict #13826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7680c19
FIX: restore de-confliction logic for minor ticks
tacaswell Apr 1, 2019
6642990
ENH: Add API from minor locators to opt-out of tick deconfliction
tacaswell Mar 31, 2019
62838ad
MNT: only do filtering if we need to
tacaswell Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 12 additions & 8 deletions lib/matplotlib/axis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,19 +1064,18 @@ def _update_ticks(self):
Update ticks (position and labels) using the current data interval of
the axes. Return the list of ticks that will be drawn.
"""

major_locs = self.major.locator()
major_locs = self.get_majorticklocs()
major_labels = self.major.formatter.format_ticks(major_locs)
major_ticks = self.get_major_ticks(len(major_locs))
self.major.formatter.set_locs(major_locs)
major_labels = self.major.formatter.format_ticks(major_locs)
for tick, loc, label in zip(major_ticks, major_locs, major_labels):
tick.update_position(loc)
tick.set_label1(label)
tick.set_label2(label)
minor_locs = self.minor.locator()
minor_locs = self.get_minorticklocs()
minor_labels = self.minor.formatter.format_ticks(minor_locs)
minor_ticks = self.get_minor_ticks(len(minor_locs))
self.minor.formatter.set_locs(minor_locs)
minor_labels = self.minor.formatter.format_ticks(minor_locs)
for tick, loc, label in zip(minor_ticks, minor_locs, minor_labels):
tick.update_position(loc)
tick.set_label1(label)
Expand Down Expand Up @@ -1317,16 +1316,21 @@ def get_minorticklocs(self):
# Remove minor ticks duplicating major ticks.
major_locs = self.major.locator()
minor_locs = self.minor.locator()
# we do check when we set the attribute that this is a Locator
# subclass, use getattr out of an over abundance of caution
remove_overlaps = getattr(self.minor.locator, 'remove_overlaps', True)
Copy link
Member

Choose a reason for hiding this comment

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

OK, so you went w/ the attribute path? So, how does this get set? I guess this PR needs tests and examples...

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.

While we are here, I think the tolerance should be smaller here. .... 1.0001 == 1.000001 with this.... I would tend to go w/ something like 1e-10.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in pixel space though. If the goal is to prevent tick/grid/label collisions shouldn't we make this closer to 1?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a visual issue, but rather a math one. If the user wants a major tick at 1.0 and a minor tick at 1.00001 I don't think we should stop them (or make them use a kwarg) short of numerical roundoff.

Copy link
Member Author

Choose a reason for hiding this comment

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

All of the complaints about this have been due to visual overlap.

The problem we have is that we can not tell "the user intentionally put a major and minor ticks on top of each other so we should do what we are told" (ex, @ImportanceOfBeingErnest date example) vs "the user accidentally set up locators that collide and we should do the obviously correct thing and de-conflict them"

Previously the behavior was the first (which when we made the log minor locator smarter caused problems and @ImportanceOfBeingErnest was relying on) but now we do the second. If we accept that we want to de-conflict the ticks as a primarily a display issue then we should do the filtering based on how close it comes in pixel space.

Using this flag the user can pick between the two behavior (by telling us which of the two cases they want) and we chose to default to door number 2 ("do 'the right thing' visually) as I think that is going to be the more common intention.

Copy link
Member

Choose a reason for hiding this comment

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

Thats fine, I understand your point. However:

then we should do the filtering based on how close it comes in pixel space.

I don't think we should do any deciding of what to send to the renderer in pixel space because that means a 100-dpi figure is different than 200 dpi figure. You will end up with the inconsistent situation that a pdf that has 72 dpi virtual pixels will have fewer ticks than a png with 100 dpi.

If your argument were instead to do the deconflict as a fraction of tick width, I'd be fine w/ that. linewidths are in physical units.

Anyway, this comment wasn't meant at all to be a blocker on this PR. The overall idea seems fine to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

The DPI dependence is a very good point. I would be comfortable tweaking the exact de-confliction criteria later.

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()]
if remove_overlaps:
minor_locs = [
loc for loc, tr_loc in zip(minor_locs, tr_minor_locs)
if ~(np.isclose(tr_loc, tr_major_locs, atol=tol, rtol=0).any())
]
return minor_locs

def get_ticklocs(self, minor=False):
Expand Down
5 changes: 5 additions & 0 deletions lib/matplotlib/ticker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1469,6 +1469,11 @@ class Locator(TickHelper):
# many ticks are generated.
MAXTICKS = 1000

# Default to requsting minor locators have overlapping ticks with the
# major locator removed. If you want to have overlapping ticks, set
# this to False on the Locator instance.
remove_overlaps = True

def tick_values(self, vmin, vmax):
"""
Return the values of the located ticks given **vmin** and **vmax**.
Expand Down