Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Make LogLocator only return one tick out of range#24436
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 |
46cb27a tof329260CompareThis is good for review now - I changed to logic a bit (see changelog entry) and updated the docstring of |
Uh oh!
There was an error while loading.Please reload this page.
The PR title is no longer correct now, I think? |
Ah thanks for catching - I've updated it 👍 |
| 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. |
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.
Can this clarify the previous behaviour? Did we un-conditionally return an extra tick?
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.
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.
| 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 |
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.
Was this comment wrong before?
| @@ -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 hasnopositive values, and therefore cannot be " | |||
| "Data hasnon-positive values, and therefore cannot be " | |||
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.
| "Data hasnon-positive values, and therefore cannot be " | |
| "Data hasnopositive 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.
| @@ -2359,7 +2368,7 @@ def tick_values(self, vmin, vmax): | |||
| if vmin <= 0.0 or not np.isfinite(vmin): | |||
| raise ValueError( | |||
| "Data hasnopositive values, and therefore can not be " | |||
| "Data hasnon-positive values, and therefore can not be " | |||
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.
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.
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 exactlyat 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 commentedFeb 3, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
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.
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 😃 |
Uh oh!
There was an error while loading.Please reload this page.
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 abovevmax. 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 belowvmin/one ablovevmax(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
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst