Skip to content

Allow invalid limits when panning #9363

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 23, 2017
Merged

Allow invalid limits when panning #9363

merged 2 commits into from
Oct 23, 2017

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 11, 2017

PR Summary

Fixes #9361. @2sn please give it a try?
The "culprit" was #8474... this PR adds an escape hatch.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 11, 2017
@anntzer anntzer added this to the 2.1.1 (next bug fix release) milestone Oct 11, 2017
@anntzer anntzer mentioned this pull request Oct 11, 2017
Closed
@2sn
Copy link

2sn commented Oct 11, 2017

I would be happy to try this, alone I cannot find any instructions how to get this change into the maplotlib git repo I just downloaded for 30 min. I am sure it is easy to do, but I cannot guess how to. There no git has provided, etc. If it is even in the git main repo ... sorry, I just have never used this.

@2sn
Copy link

2sn commented Oct 11, 2017

I managed to get you changes on top of current master. No improvement. I firmly believe all "upgrades" on this should just be undone until this can get under control. Here the error message I get from my code which is too long to share:

In [3]: Traceback (most recent call last):
  File "/home/alex/matplotlib/lib/matplotlib/cbook/__init__.py", line 389, in process
    proxy(*args, **kwargs)
  File "/home/alex/matplotlib/lib/matplotlib/cbook/__init__.py", line 227, in __call__
    return mtd(*args, **kwargs)
  File "/home/alex/matplotlib/lib/matplotlib/backend_bases.py", line 3116, in release_zoom
    self._zoom_mode, twinx, twiny)
  File "/home/alex/matplotlib/lib/matplotlib/axes/_base.py", line 3650, in _set_view_from_bbox
    self.set_xlim((x0, x1))
  File "/home/alex/matplotlib/lib/matplotlib/axes/_base.py", line 2932, in set_xlim
    self.viewLim, "intervalx", emit, auto, kw)
  File "/home/alex/matplotlib/lib/matplotlib/axes/_base.py", line 2842, in _set_lim
    "Axis limits must be (or convert to) finite reals")
ValueError: Axis limits must be (or convert to) finite reals

@2sn
Copy link

2sn commented Oct 11, 2017

The other problem is that I have a user-created axis that does not plot at all in mpl 2.1, and there is not even an error message. I believe this is due to some silencing of error messages that was also done in 2.1 for the sake of, aka matplotlib.cbook.CallbackRegistry.process() suppresses exceptions by default as listed on the API changes. It remains unmentioned how or where I can influence this as a user.

@2sn
Copy link

2sn commented Oct 11, 2017

OK, I could find the reason for one of the issues, the behaviour of of ax.transData has changed from 2.0.2 to 2.1. If y-axis is log and a y-value of 0 is provided, it returns np.nan also for the x-value. This is a bug and should be fixed. Probably a separate bug report should be filed?

@2sn 2sn mentioned this pull request Oct 11, 2017
Closed
@2sn
Copy link

2sn commented Oct 11, 2017 •
edited
Loading

After identifying #9369 your patch seems to fix my problem. Thanks! When this is included, I assume you may close #9361.

dstansby
dstansby reviewed Oct 12, 2017
View reviewed changes
lib/matplotlib/axes/_base.py Outdated
return converted_limit
def _set_lim(self, axis, low, high, low_name, high_name,
target_obj, target_attr, emit, auto, kw):
"""Helper factoring the functionality of `get_{x,y,z}lim`."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a private method, but I still think it would be good to write quick one line descriptions of all the args.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dstansby
dstansby reviewed Oct 12, 2017
View reviewed changes
lib/matplotlib/axes/_base.py Outdated
"""Helper factoring the functionality of `get_{x,y,z}lim`."""
# Perhaps we can use axis.{get,set}_view_interval()... but zaxis seems
# to behave differently.
_called_from_pan = kw.pop("_called_from_pan", False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be clearer to just add called_from_pan as it's own kwarg with a default given it's the only kwarg, then the next two lines aren't needed to check for extra kwargs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I decided to just do the check from the drag_pan function rather than trying to pass it as a private kwarg throught the stack...

@anntzer anntzer force-pushed the ignore-invalid-limits-in-pan branch 3 times, most recently from 9a7a247 to 05e383a Compare October 12, 2017 20:54
@anntzer
Copy link
Contributor Author

anntzer commented Oct 12, 2017

The latest version actually works better than the pre-broken version: it doesn't even bother passing invalid values to set_xlim anymore.

@tacaswell
Copy link
Member

I like the direction this is going 👍

@anntzer anntzer force-pushed the ignore-invalid-limits-in-pan branch from 05e383a to b848e8a Compare October 12, 2017 23:32
@anntzer
Copy link
Contributor Author

anntzer commented Oct 12, 2017

Turns out the set_?lim unification is a mess, because custom axes typically don't define get_${axisname}lim so you need to chase them down to see who's using what name.
Stripped that part out for now, will probably be the object of a separate PR later.
Now we still get warnings about invalid values but at least it should behave similarly to 2.0.2.

@dopplershift
Copy link
Contributor

Was the change to ban calling set_?lim with NaNs intentional? That's creating new challenges for us with real-world, real-time messy data. I've got a script that worked in 2.0 that now fails with 2.1 when we calculate limits from data that's all missing.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 17, 2017

The issue was discussed (with a quick vote...) on #7460. I don't think the previous behavior was particularly helpful, but feel free to revisit it...

@dopplershift
Copy link
Contributor

Thanks for pointing me to that...looks like I need to fix things on our end.

@tacaswell
Copy link
Member

Independent of if we should treat np.nan as None in setting the limits, this avoids the whole problem when panning.

@dstansby dstansby merged commit efbaded into matplotlib:master Oct 23, 2017
lumberbot-app bot pushed a commit that referenced this pull request Oct 23, 2017
@dstansby
Backport PR #9363: Allow invalid limits when panning
4b872b1
@lumberbot-app lumberbot-app bot mentioned this pull request Oct 23, 2017
Merged
@anntzer anntzer deleted the ignore-invalid-limits-in-pan branch October 23, 2017 09:31
tacaswell added a commit that referenced this pull request Oct 23, 2017
@tacaswell
Merge pull request #9539 from matplotlib/auto-backport-of-pr-9363
f39813c 
Backport PR #9363 on branch v2.1.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v2.1.1
Development

Successfully merging this pull request may close these issues.

2.1 change - Axis Limit Error
5 participants
@anntzer @2sn @tacaswell @dopplershift @dstansby