Skip to content

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

Merged
merged 3 commits into from
Oct 2, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 12, 2019

PR Summary

Closes #15251; see discussion there.

Edit: Also closes #15331.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak
Copy link
Member

jklymak commented Sep 13, 2019

I pushed an extra comment.

Like this is fine for now, because I agree it was how autoscale_view is supposed to work the way the code is written now.

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.

@dopplershift
Copy link
Contributor

@anntzer Did you check it with the test code in #15251? If I locally check out your PR, I still see the large memory growth.

@dopplershift
Copy link
Contributor

When fixing the issue, this comment should probably be updated:

# Some automatic tick locators can generate so many ticks they
# kill the machine when you try and render them.
# This parameter is set to cause locators to raise an error if too
# many ticks are generated.
MAXTICKS = 1000

because it no longer raises any more, which confused me.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 13, 2019

oops.
I checked @jklymak's example where just yscale("log") would result in huge limits -- this PR fixes that, with now a test; but not yours. Indeed things still break in your case because you swap out the locator before it had time to set proper limits; I guess this validates @jklymak's point that this should really be the job of the scale, not of the locator. In the meantime, though, this can be fixed by forcing an autoscale immediately after set_xscale/set_yscale...

@jklymak
Copy link
Member

jklymak commented Sep 13, 2019

Like I guess this is fine. Is there a reason to not just do a _request_autoscale_view like we do for adding most artists?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 13, 2019

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).

@jklymak
Copy link
Member

jklymak commented Sep 13, 2019

Sorry I meant where you put the extra logic in scale.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 13, 2019

sorry, what do you exactly propose?

@jklymak
Copy link
Member

jklymak commented Sep 13, 2019

In set_yscale, just call self.autoscale_view (I thought you could say "request" but it doesn't get called until too late as you say). That just seems simpler than a the convoluted way you are checking to inf now.

I still think this should be in scale We already have limit_range_for_scale; a default_range_for_scale would make a heck of a lot more sense than calling nonsingular for the locator. (Why is it called nonsingular anyhow?)

@anntzer
Copy link
Contributor Author

anntzer commented Sep 13, 2019

In set_yscale, just call self.autoscale_view (I thought you could say "request" but it doesn't get called until too late as you say). That just seems simpler than a the convoluted way you are checking to inf now.

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...

I still think this should be in scale We already have limit_range_for_scale; a default_range_for_scale would make a heck of a lot more sense than calling nonsingular for the locator.

Agreed this would be a better place (I forgot about limit_range_for_scale), but then where do you handle dates?

(Why is it called nonsingular anyhow?)

Probably because it (also) expands singular limits (xmin=xmax)?

@jklymak
Copy link
Member

jklymak commented Sep 13, 2019

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

@anntzer
Copy link
Contributor Author

anntzer commented Sep 13, 2019

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 :)
I guess alternatively units could grow their own limit_range_for_unit and we'd call both the scale's limit_range_for_scale and the unit's limit_range_for_unit in some yet to be decided order. "Blech", as you say.

@dopplershift
Copy link
Contributor

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.

@jklymak
Copy link
Member

jklymak commented Sep 16, 2019

The practical problem is that the default linear limits are [0, 1], and the default datetime library doesn't accept anything less than 1.0 as an ordinal number. So if someone instantiates a date axes, but don't put any data on it, it errorred out.

How the locators ended up as the home of the default limits, I don't know.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 17, 2019

By the way, note that limit_range_for_scale "doesn't really work", either -- it takes 3 args -- vmin, vmax, and minpos, where minpos (the smallest positive data value) is basically there "just" so that log scale can do its thing, but of course other scales such as logit are basically stuck because we don't track the largest data value less than 1...
I think the correct approach is actually to not track dataLim as artists are being added, but rather, at autoscaling time, perform the transform and compute the (finite) data limits from there -- then negative values in log scale just map to nan and can be properly ignored. Unfortunately this runs into a snag when log scale uses nonpos="clip" rather than nonpos="mask" :(

Copy link
Member

@jklymak jklymak left a 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?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 28, 2019

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.

@anntzer anntzer added this to the v3.2.0 milestone Sep 28, 2019
@anntzer
Copy link
Contributor Author

anntzer commented Sep 28, 2019

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...

@ivanov ivanov merged commit 1dc52df into matplotlib:master Oct 2, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 2, 2019
@anntzer anntzer deleted the nonsingular branch October 2, 2019 20:50
dstansby added a commit that referenced this pull request Oct 2, 2019
…258-on-v3.2.x

Backport PR #15258 on branch v3.2.x (Don't fallback to view limits when autoscale()ing no data.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants