Make LogLocator only return one tick out of range #24436

dstansby · 2022-11-12T10:27:24Z

PR Summary

I'm working on thoroughly testing LogLocator, and this PR is a start. There is a new parametrized test that checks the tick locations for a given vmin, vmax.

I have also changed the logic for geting the tick locations slightly. Before the locator was returning two ticks below vmin, and two ticks above vmax. Looking through the commit history I cannot find any reason to do this, so to make the logic clear I have changed this to returning one tick below vmin/one ablove vmax (as is needed for clipping and potentially autoscaling). I've then documented this logic in the docstring.

The changed tests are just removing the extra tick at each end - because there is still a single "out of bounds" tick returned at each end no figures should change with this PR.

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

Documentation is sphinx and numpydoc compliant (the docs should build without error).
New plotting related features are documented with examples.

Release Notes

New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

QuLogic · 2022-11-24T06:48:32Z

I think it's been a long-standing confusion that locators don't have to return tick locations in (vmin, vmax). I'm not sure, but this might not be specific to LogLocator.

dstansby · 2022-12-01T11:12:23Z

This is good for review now - I changed to logic a bit (see changelog entry) and updated the docstring of tick_values to match.

lib/matplotlib/tests/test_ticker.py

QuLogic · 2022-12-15T21:15:31Z

The PR title is no longer correct now, I think?

dstansby · 2022-12-15T21:20:39Z

Ah thanks for catching - I've updated it 👍

jklymak · 2023-02-02T17:40:47Z

doc/api/next_api_changes/behavior/24436-DS.rst

+for the decades containing ``vmin`` and ``vmax``. If ``vmin`` or ``vmax``
+are exactly on a decade this means no ticks above/below are returned.
+Otherwise a single major tick is returned above/below, and for minor ticks
+all the ticks in the decade containing ``vmin`` or ``vmax`` are returned.


Can this clarify the previous behaviour? Did we un-conditionally return an extra tick?

Before it was 1 or 2, now it is 0 or 1.

I think the question is MUST locators return extra or MAY locators return extra.

tacaswell · 2023-02-02T18:35:17Z

lib/matplotlib/tests/test_colorbar.py

-                                       10**np.arange(-16., 16.2, 4.))
-        # note only -24 to +24 are visible
+                                       10**np.arange(-12., 12.2, 4.))
+        # note only -12 to +12 are visible


Was this comment wrong before?

tacaswell · 2023-02-02T18:42:42Z

lib/matplotlib/ticker.py

@@ -2450,7 +2461,7 @@ def nonsingular(self, vmin, vmax):
            vmin, vmax = 1, 10  # Initial range, no data plotted yet.
        elif vmax <= 0:
            _api.warn_external(
-                "Data has no positive values, and therefore cannot be "
+                "Data has non-positive values, and therefore cannot be "


Suggested change

"Data has non-positive values, and therefore cannot be "

"Data has no positive values, and therefore cannot be "

I think this one should stay as "no positive" as in the other blocks we clip vmin to a small positive value.

tacaswell · 2023-02-02T18:44:31Z

lib/matplotlib/ticker.py

@@ -2359,7 +2368,7 @@ def tick_values(self, vmin, vmax):

            if vmin <= 0.0 or not np.isfinite(vmin):
                raise ValueError(
-                    "Data has no positive values, and therefore can not be "
+                    "Data has non-positive values, and therefore can not be "


This overall logic does not seem 10% correct because we are only checking vmin here and just below we ensure that vmin and vmax are sorted.

We also only get this error if the locator is not attached to as axis as in that case we silently clip vmin.

tacaswell · 2023-02-02T19:12:27Z

My understanding is that locators MAY return at least 1 "extra" tick on either side to deal with clipping / float rounding issues when there might be a tick exactly at vmin or vmax and we want to defer the clipping to draw time (both to be safe and to only have one place to go hunting for the bug).

The weird thing about LogLocator is that we get one expansion from the floor / ceiling logic and the a second from the "lets expand" which means in the general case we get 2 "extra" locations (the decade bracketing the vmin/vmax and the next one). However, I think the "special case" of vmin / vmax already being on a decade are far more likely due to conventions (at least the field I came out of) for always putting the view limits of a log axis to be on a decade so I think this is pushing us to the state of "no extra" in the most common case.

My risk calculus is that we have had things go wrong with too few ticks and I can not think of something that would go wrong with too many. Hence, I am wary of doing this trimming but not enough that I would block it.

dstansby · 2023-02-03T17:20:36Z

The weird thing about LogLocator is that we get one expansion from the floor / ceiling logic and the a second from the "lets expand"

I don't think this is right - we were getting two ticks just from this line (before this PR):

decades = np.arange(math.floor(log_vmin) - stride,
                    math.ceil(log_vmax) + 2 * stride, stride)

e.g. if log_vmin = 1.5 and stride=1, this will have ticks at log values of 0, 1, 2..., where 0 and 1 are out of bounds.

I think the question is MUST locators return extra or MAY locators return extra.

Agreed. I'm not 100% sure on the answer to this, but would say that to be safe MUST return one extra seems sensible. I'll open a new issue to track deciding and documenting this.

My risk calculus is that we have had things go wrong with too few ticks and I can not think of something that would go wrong with too many.

The motivation for this PR was really to reduce internal tech debt I guess - returning one or two ticks didn't make sense to me. But I do agree that it's probably safer to leave this alone, so I will close this PR (and resurrect the testing bits without changing the logic). Thanks all for taking the time to carefully look over this 😃

github-actions bot added the status: needs rebase label Nov 17, 2022

dstansby mentioned this pull request Nov 30, 2022

[Bug]: get_ticklocs() returns ticks outside axis limits for log scale #24572

Closed

dstansby force-pushed the test-loglocator branch from 79807c3 to 0e0de00 Compare November 30, 2022 22:52

github-actions bot removed the status: needs rebase label Nov 30, 2022

dstansby force-pushed the test-loglocator branch 2 times, most recently from 46cb27a to f329260 Compare December 1, 2022 11:04

dstansby marked this pull request as ready for review December 1, 2022 11:12

dstansby mentioned this pull request Dec 1, 2022

Make LogLocator respect numticks #21177

Closed

3 tasks

dstansby added 4 commits December 1, 2022 11:16

Move invalid vmin to separate test

d495301

Add some new LogLocator tests

5e733de

Add note about out of bounds ticks

4ba7411

Simplify log locator logic

737aa02

dstansby force-pushed the test-loglocator branch from 2770b30 to 795bf97 Compare December 1, 2022 11:16

dstansby added 2 commits December 1, 2022 12:05

Fix tests

36adb34

Add changelog entry

792236a

dstansby force-pushed the test-loglocator branch from 795bf97 to 792236a Compare December 1, 2022 12:06

timhoffm reviewed Dec 1, 2022

View changes

lib/matplotlib/tests/test_ticker.py Outdated Show resolved Hide resolved

Fix non-positive warning

a8ba49d

dstansby changed the title ~~Add some more LogLocator tests~~ Make LogLocator only return one tick out of range Dec 15, 2022

dstansby added this to the v3.8.0 milestone Jan 8, 2023

jklymak reviewed Feb 2, 2023

View changes

tacaswell reviewed Feb 2, 2023

View changes

dstansby closed this Feb 3, 2023

dstansby mentioned this pull request Feb 3, 2023

[Doc]: Document how many out of bounds ticks Locators should return #25146

Open

Make LogLocator only return one tick out of range #24436

Make LogLocator only return one tick out of range #24436

dstansby commented Nov 12, 2022 •

edited

QuLogic commented Nov 24, 2022

dstansby commented Dec 1, 2022

QuLogic commented Dec 15, 2022

dstansby commented Dec 15, 2022

jklymak Feb 2, 2023

tacaswell Feb 2, 2023

tacaswell Feb 2, 2023

tacaswell Feb 2, 2023

tacaswell Feb 2, 2023

tacaswell commented Feb 2, 2023

dstansby commented Feb 3, 2023 •

edited

	"Data has non-positive values, and therefore cannot be "
	"Data has no positive values, and therefore cannot be "

Make LogLocator only return one tick out of range #24436

Make LogLocator only return one tick out of range #24436

Conversation

dstansby commented Nov 12, 2022 • edited

PR Summary

PR Checklist

QuLogic commented Nov 24, 2022

dstansby commented Dec 1, 2022

QuLogic commented Dec 15, 2022

dstansby commented Dec 15, 2022

jklymak Feb 2, 2023

Choose a reason for hiding this comment

tacaswell Feb 2, 2023

Choose a reason for hiding this comment

tacaswell Feb 2, 2023

Choose a reason for hiding this comment

tacaswell Feb 2, 2023

Choose a reason for hiding this comment

tacaswell Feb 2, 2023

Choose a reason for hiding this comment

tacaswell commented Feb 2, 2023

dstansby commented Feb 3, 2023 • edited

dstansby commented Nov 12, 2022 •

edited

dstansby commented Feb 3, 2023 •

edited