Skip to content

Deprecate MAXTICKS, Locator.raise_if_exceeds. #8100

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

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 19, 2017

cf #8089.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Feb 20, 2017
@tacaswell
Copy link
Member

I am conflicted on this.

This limit came in via 0022de2 (and the comment says it is to prevent out of memory).

On one hand, we have had several reports people running into this limit on machines that can handle it and we should not be putting artificial limits on things.

On the other hand, if we raise, users can in principle recover where as if we just let things run out of memory they can not.

On the third hand, rendering more than 1k ticks to the raster formats gets silly, there just are not enough pixels in most cases (and won't go so well with the vector formats either).

I think the option here are:

  1. do nothing to code, but document the limit better
  2. drop limit all together (this PR)
  3. bump up limit (only kicks can down the road)
  4. make limit 'smart' based on current figure size + dpi ( + memory?!) which is probably more complicated than we want to deal with.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 20, 2017

On the other hand, if we raise, users can in principle recover where as if we just let things run out of memory they can not.

Actually they can't even catch it (I think) because the exception is raised in the GUI thread during draw_idle (try putting try... except RuntimeError around plt.show and see it doesn't work). Additionally the exception is raised again when you click with the mouse (pick -> get_children -> get_major_ticks), which leads to a fatal, aborting error on the Qt5 backend at least (cf #7197).

make limit 'smart' based on current figure size + dpi ( + memory?!) which is probably more complicated than we want to deal with.

We don't do anything for line plots or images (for which there's also a point of diminishing returns), I don't see why tickers should be different.

@tacaswell
Copy link
Member

They can catch it in the context of fig.savefig, I am more worried about non-interactive things silently dying rather vs interactive things.

The difference between user supplied data and ticks is that if they hand us large volumes, it is on them to know what they are doing, but with ticks we do a lot more 'automagically' so need to be more careful.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 20, 2017

Fair enough. I have a strong suspicion that the number of people currently doing

try: fig.savefig(...)
except RuntimeError: ...

is pretty low though :-)

@efiring
Copy link
Member

efiring commented Feb 20, 2017

I like option 1. The limit was put in for a perfectly good reason, based on experience that is still relevant.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 20, 2017

As mentioned above, the limit can only help if one is using something like

try: fig.savefig(...)
except RuntimeError: ...

(afaict, let me know if I missed other possibilities) and actually has a very negative impact (program abort) for programs using the Qt5 backend.

As I said, I doubt there's much code in the wild using that pattern (if you're aware of that possibility you may as well make sure you won't trigger that code path by setting the tickers properly to start with). Unless you can prove me wrong, I would still favor removing the limit.

@anntzer anntzer force-pushed the deprecate-MAXTICKS branch from af930ce to 565d83f Compare March 28, 2017 05:19
@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017
@anntzer anntzer force-pushed the deprecate-MAXTICKS branch from 565d83f to 373b965 Compare November 4, 2017 06:43
@anntzer anntzer modified the milestones: needs sorting, v3.0 Feb 26, 2018
@tacaswell tacaswell modified the milestones: v3.0, v3.1 Jul 15, 2018
@tacaswell tacaswell modified the milestones: v3.1.0, v3.2.0 Feb 24, 2019
@tacaswell
Copy link
Member

Still have not reached consensus on this, punting to 3.2

@tacaswell
Copy link
Member

This does couple into the more recent work with logging, this may be a case where we want to change exceptions into log messages?

@anntzer
Copy link
Contributor Author

anntzer commented Mar 1, 2019

Closed in favor of #13510.

@anntzer anntzer closed this Mar 1, 2019
@anntzer anntzer deleted the deprecate-MAXTICKS branch March 1, 2019 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants