| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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: diff --git i/lib/matplotlib/tests/test_tightlayout.py w/lib/matplotlib/tests/test_tightlayout.py
index 5898c1a94..a8f833f07 100644
--- i/lib/matplotlib/tests/test_tightlayout.py
+++ w/lib/matplotlib/tests/test_tightlayout.py
@@ -167,16 +167,14 @@ def test_outward_ticks():
'Test automatic use of tight_layout'
fig = plt.figure()
ax = fig.add_subplot(221)
- ax.xaxis.set_tick_params(tickdir='out', length=16, width=3)
- ax.yaxis.set_tick_params(tickdir='out', length=16, width=3)
- ax.xaxis.set_tick_params(
- tickdir='out', length=32, width=3, tick1On=True, which='minor')
- ax.yaxis.set_tick_params(
- tickdir='out', length=32, width=3, tick1On=True, which='minor')
- # The following minor ticks are not labelled, and they
- # are drawn over the major ticks and labels--ugly!
- ax.xaxis.set_ticks([0], minor=True)
- ax.yaxis.set_ticks([0], minor=True)
+ for axis in [ax.xaxis, ax.yaxis]:
+ axis.set_ticks([0.2, 0.4, 0.6, 0.8, 1.0])
+ axis.set_tick_params(tickdir='out', length=16, width=3)
+ axis.set_ticks([0], minor=True)
+ # The following minor ticks are not labelled, and they
+ # are drawn over the major ticks and labels--ugly!
+ axis.set_tick_params(
+ which='minor', tick1On=True, tickdir='out', length=32, width=3)
ax = fig.add_subplot(222)
ax.xaxis.set_tick_params(tickdir='in', length=32, width=3)
ax.yaxis.set_tick_params(tickdir='in', length=32, width=3)
) 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Adding a remove_ticks=... to __call__ means going through a deprecation to not break third-party locators (ugh), and likely a bunch of locators will need to basically use the same code anyways (well, that can always be factored out in a helper method, but still...) 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.) |
Sorry, something went wrong.
|
Oh, duh, you are right - I thought calling foo(unknown_kwarg='Boo') just ignored the kwarg, but thats only if the call signature has **kwargs which locator.__call__ doesn't have. Now that I think of it, allowing API changes to have better backwards compatibility is actually a pretty good reason to have signatures with **kwargs. |
Sorry, something went wrong.
|
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 try block, but that is inelegant. |
Sorry, something went wrong.
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 **kwargs as they would work fine without that.
Should probably have a look at what adhoc mechanisms are currently used and how this PR could or could not subsume them. |
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
|
No I don’t think so, but you could imagine doing something like that.... |
Sorry, something went wrong.
|
Then I think I'd rather leave this PR as it is and worry about changing the signature of __call__ when an actual use case shows up... |
Sorry, something went wrong.
|
Edited the other failing test (initially added in #5683) to check the axes positions instead. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
|
Thinking again about adding remove_ticks to __call__ as a mechanism for inter-locator communication, I think that basically assumes (*) that the minor locator would always be used with the "corresponding" major locator (so that the minor locator can meaningfully interpret what is pass down to it). It is true that in general, a minor loglocator is used with a major loglocator (for example). But if you look at the issue that I mentioned in this PR, some of them come from people forcing major tick locations (effectively using a FixedLocator under the hood) and expecting the minor ticker to know not to put a tick there, so the assumption (*) does not hold. 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 (self is self.axis.minor.locator) and then 2) call the major locator again to see what it outputs (self.axis.major.locator()). |
Sorry, something went wrong.
|
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 major_ticks and then that is the signal that the locator is a minor locator. |
Sorry, something went wrong.
|
Again, I think we should have an actual example where passing the major ticks down is useful before going through the API change pain. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Right now, the locators need to know what the axis is because when you call locator(), they need to look up the axis limits; they also use it to consult axis.get_tick_space() (for determining the number of ticks). 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Just commenting - these aren't necessarily blockers from my point of view, but should be discussed before I approve....
Sorry, something went wrong.
| 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.
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...
Sorry, something went wrong.
There was a problem hiding this comment.
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.).
Sorry, something went wrong.
| """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.
aren't we now calling both locators twice? Seems inefficient to me.
Sorry, something went wrong.
There was a problem hiding this comment.
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...
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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 ( :) ).
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| 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.
"transformed view limits" means this is now in axes coordinates, right? If so, this is the right thing to do.
Sorry, something went wrong.
There was a problem hiding this comment.
It's some linear transform away from axes coordinates, so the relative check is equivalent to being done in axes coordinates.
Sorry, something went wrong.
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.
|
rebased |
Sorry, something went wrong.
|
Anyone can merge after CI pass. |
Sorry, something went wrong.
|
azure failure was download related, restarting. |
Sorry, something went wrong.
…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
| Back | FazBrowse Home | New Git URL |
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