-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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.
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
Conversation
35c95de
to
a67157d
Compare
The test_tightlayout.py::test_outward_ticks failure is explicitly testing that one can overlay a minor tick on top of a major tick, so I'll probably just remove that test (after looking into whether it is testing anything else relevant) (The patch below does not help because the test even requires a major label at x=0 that's overstruck by a minor tick:
) The test_scale.py::test_logscales failure is (I believe?) due to the fact that the Symlog minor locator previously did not implement overstriking avoidance, so we ended up with overstruck major and minor ticks, which the svg rasterizer would pick up (one can examine the svg output to confirm); with this PR, the overstruck minor tick is absent, causing a tiny change in rasterization. Again, may be worth just killing the test if it's not testing anything else (after proper investigation). Edit: test_logscales was introduced in #1247 to check that ax{v,h}line works with (sym)log scales. Rewrote it to use check_figures_equal. |
Overall 👍 But another approach would have been to add a remove_ticks kwarg to locator.call. Not sure which approach is better but this way the locator could decide what to do with the ticks to remove. It’s also possible that the locator logic could do something interesting with the major ticks info. |
Adding a remove_ticks=... to OTOH it does allow for really weird cases if you really want your custom locator to overstrike a major tick with a minor tick, not that I think there's a realistic use case... (the test_outwards_tick test is completely artificial.) |
11394cf
to
638bdd0
Compare
Oh, duh, you are right - I thought calling Now that I think of it, allowing API changes to have better backwards compatibility is actually a pretty good reason to have signatures with |
For the "weird" cases, I could imagine knowing the major ticks could change the minor tick intervals. i.e. for dates, if you know the major ticks are days, making the minor ticks hours is useful. No doubt we already have logic for that, but it would be made easier by knowing the major ticks. I guess compatibility could be maintained by a |
That doesn't really help: for our own locators we can update them as desired, for third party locators we can't really force them to accept
Should probably have a look at what adhoc mechanisms are currently used and how this PR could or could not subsume them. |
@jklymak I skimmed through dates.py and didn't see anything noticeable. Do you know of any example with interesting interactions between major and minor dates locators? |
No I don’t think so, but you could imagine doing something like that.... |
Then I think I'd rather leave this PR as it is and worry about changing the signature of |
Edited the other failing test (initially added in #5683) to check the axes positions instead. |
Missed the fact that test_symlog{,2} suffers from the same overstriking problem as test_logscales. Dropped the png and svg tests, and kept the pdf baseline, as ghostscript appears not to pick up the overstriking, so the fundamental functionality (of symlog scales) is still tested. |
I don't think there is a compelling need to have those tests make pdfs and svgs. OTOH, I bet the png still won't match because of anti-aliasing so maybe update the baseline image? |
the png also fails due to antialiasing (likely), it is the pdf test that I kept; likely it works because ghostscript's rasterizer is not as good and does not perform the antialiasing correctly? :p PS: I find it pretty funny that github's UI includes the svg files in the +/- lines count :) |
Thinking again about adding remove_ticks to If a minor locator really wants to know what a major locator did, it should probably(?) do something like 1) check that itself is the minor locator ( |
I don't follow that argument. Why would it assume anything about what kind of locator it is or if its been used with a major locator? Its just saying: don't use these possible tick values, and maybe being clever about what to do between those tick values. If you like, we could call it |
Again, I think we should have an actual example where passing the major ticks down is useful before going through the API change pain. |
OK, getting in the weeds, but why does the locator care about its axis? We pass stuff around on a somewhat ad-hoc basis and this is one case where I'm not sure what the benefit is. |
Right now, the locators need to know what the axis is because when you call The formatters need to know what the axis is to consult the axis limits. I would like to make tickers independent of the axis (and always pass the axis down as argument) to make them reusable over multiple axises, but that's not going to happen here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just commenting - these aren't necessarily blockers from my point of view, but should be discussed before I approve....
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() |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.).
"""Get the array of minor tick locations in data coordinates.""" | ||
# Remove minor ticks duplicating major ticks. | ||
major_locs = self.major.locator() | ||
minor_locs = self.minor.locator() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ( :) ).
532ad38
to
d953c0b
Compare
Tentatively milestoning as 3.1 as this already has one positive review (and fixes a longstanding bug), feel free to remilestone if it doesn't make it. |
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The Axis knows what the major and the minor ticker are so it makes sense to let it handle the deduplication logic. Revert some heuristic deduplication logic from the Locators themselves (which effectively tests that the new code works).
Its original intent was to check that ax{v,h}line works in (sym)log scales, and the baseline svg changed with the removal of minor tick overstriking.
d953c0b
to
a752d46
Compare
rebased |
Anyone can merge after CI pass. |
azure failure was download related, restarting. |
…314-on-v3.1.x Backport PR #13314 on branch v3.1.x (Move major/minor tick overstrike logic to Axis.)
In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.
In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.
If Axis.remove_overlapping_locs is set to False the `Axis` object will not attempt to remove locations in the minor ticker that overlap with locations in the major ticker. This is a follow up to matplotlib#13314 which un-conditionally removed the overlap. closes matplotlib#13618
In matplotlib#13363 when `iter_ticks` was deprecated the in-lined logic did not account for the updates from matplotlib#13314.
If Axis.remove_overlapping_locs is set to False the `Axis` object will not attempt to remove locations in the minor ticker that overlap with locations in the major ticker. This is a follow up to matplotlib#13314 which un-conditionally removed the overlap. closes matplotlib#13618
PR Summary
The Axis knows what the major and the minor ticker are so it makes sense
to let it handle the deduplication logic.
Revert some heuristic deduplication logic from the Locators themselves
(which effectively tests that the new code works). (#11762, #13126)
Closes #11575 (which has a few duplicates).
Goes on top of #12909 (which has already three approvals). [edit: that got merged]
PR Checklist