-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Don't fallback to view limits when autoscale()ing no data. #15258
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
I pushed an extra comment. Like this is fine for now, because I agree it was how But.... I really don't understand why it is the locator that is doing this. It should be the scale that sets an appropriate default limits for itself. |
When fixing the issue, this comment should probably be updated: matplotlib/lib/matplotlib/ticker.py Lines 1656 to 1660 in 2aba5aa
because it no longer raises any more, which confused me. |
347d156
to
9d13b2d
Compare
oops. |
9d13b2d
to
35e30d0
Compare
35e30d0
to
63454db
Compare
Like I guess this is fine. Is there a reason to not just do a |
Because if one does so, the autoscale() occurs too late -- @dopplershift already swapped out the correct locator at that point (correct for the purpose of finding the default range). |
Sorry I meant where you put the extra logic in scale. |
sorry, what do you exactly propose? |
In I still think this should be in |
It looks this this would work, but in that case you get again a warning when using secondary axes with 1/x (because it would try to do 1/0, even if you later move the limits away from zero). Yes, it's kind of a gross hack, essentially relying on the fact that SecondaryAxes doesn't override nonsingular...
Agreed this would be a better place (I forgot about limit_range_for_scale), but then where do you handle dates?
Probably because it (also) expands singular limits (xmin=xmax)? |
OK so an axis has a scale, units, and Locators, and they are all kind of separate but intermingled. Blech. Long run I don't think dates should have a limitation to positive numbers >1 (see #15148). That will mean the default dates will be -0001-12-01 to 0000-01-01, but so be it. Do other units have strange limitations on the limits? If we agree units should not have a problem with any value thrown at them, then I'd argue the limits should still go on the scale. OTOH, I'm sure #15148 won't go in any time soon, but I think its worth thinking through where this functionality belongs |
In the core lib no other units wants their own default limits. pandas datetimes appear to use the same machinery; astropy units appear to not touch that; dunno what other big unit users there are out there that we can check. I guess we could ask @dopplershift his opinion too given that 1) he reported the original issue and 2) he's involved in units handling, IIRC :) |
In my mind the units machinery is only about converting data to a common numerical reference frame before plotting. Given that, I don't think it makes sense for limits to enter into the picture--and indeed that our current date time handling enacts limits seem kind of arbitrary. |
The practical problem is that the default linear limits are [0, 1], and the default How the locators ended up as the home of the default limits, I don't know. |
By the way, note that |
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 looks good to me as a stopgap to more major surgery. Can we get a test for the date nonsingular stuff?
They are effectively tested by test_date_empty, test_date_axhspan, test_RRuleLocator, and a couple of other tests which get broken if that snippet is removed. |
I'll milestone as 3.2 as it's a regression in 3.1, but won't insist on it as we're quite late in the cycle already... |
…scale()ing no data.
…258-on-v3.2.x Backport PR #15258 on branch v3.2.x (Don't fallback to view limits when autoscale()ing no data.)
PR Summary
Closes #15251; see discussion there.
Edit: Also closes #15331.
PR Checklist