Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ivanov merged 3 commits intomatplotlib:masterfromanntzer:nonsingular
Oct 2, 2019

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedSep 12, 2019
edited
Loading

PR Summary

Closes#15251; see discussion there.

Edit: Alsocloses#15331.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 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 commentedSep 13, 2019
edited
Loading

I pushed an extra comment.

Like this is fine for now, because I agree it was howautoscale_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
ContributorAuthor

oops.
I checked@jklymak's example where justyscale("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

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
ContributorAuthor

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

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

@anntzer
Copy link
ContributorAuthor

sorry, what do you exactly propose?

@jklymak
Copy link
Member

Inset_yscale, just callself.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 inscale We already havelimit_range_for_scale; adefault_range_for_scale would make a heck of a lot more sense than callingnonsingular for thelocator. (Why is it callednonsingular anyhow?)

@anntzer
Copy link
ContributorAuthor

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.

Itlooks 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

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
ContributorAuthor

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

The practical problem is that the default linear limits are [0, 1], and the defaultdatetime 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
ContributorAuthor

By the way, note thatlimit_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 tonot 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

@jklymakjklymak 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
ContributorAuthor

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.

@anntzeranntzer added this to thev3.2.0 milestoneSep 28, 2019
@anntzer
Copy link
ContributorAuthor

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

@ivanovivanov merged commit1dc52df intomatplotlib:masterOct 2, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestOct 2, 2019
@anntzeranntzer deleted the nonsingular branchOctober 2, 2019 20:50
dstansby added a commit that referenced this pull requestOct 2, 2019
…258-on-v3.2.xBackport PR#15258 on branch v3.2.x (Don't fallback to view limits when autoscale()ing no data.)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.

Log Scale: FloatingPointError: underflow encountered in power Large memory growth with log scaling and linear ticking
4 participants
@anntzer@jklymak@dopplershift@ivanov

[8]ページ先頭

©2009-2025 Movatter.jp