Skip to content
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

FIX: plot after contour shouldn't shrink extent #2129

Merged
merged 2 commits into from
Jan 29, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Jan 26, 2023

Rationale

Fixes #2128.

We already have the ignore=False within update_from_data_xy so that the contour call itself doesn't shrink the extent. The equivalent line for plot is

https://github.com/matplotlib/matplotlib/blob/d731d48b2431da0cf8d7edaa687f997857b112af/lib/matplotlib/axes/_base.py#L2366-L2368

which relies on ax.ignore_existing_data_limits to decide whether to shrink. Under normal circumstances, this flag is set within update_datalim, which is called by Matplotlib's contour. However, for the combination of transform and invalid latitude in the linked issue, the xys that gets passed to update_datalim is all nans.

Implications

@rcomer rcomer changed the title FIX: plot after contour shouldnt shrink extent FIX: plot after contour shouldn't shrink extent Jan 26, 2023
@rcomer
Copy link
Member Author

rcomer commented Jan 26, 2023

The Ubuntu py3.8 tests seem to be picking up an older version of proj 😕

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Thanks for the clear explanation, I think I'm following now!

Should we actually be using this Axes method instead? Which I think basically does what you're doing here.
https://github.com/matplotlib/matplotlib/blob/d772e0d4306eac3a1b14f9099711f6261153a1f0/lib/matplotlib/axes/_base.py#L2485-L2508

On the tests... :( We pin to that version of proj/pyproj, but it is odd that it is failing on only a couple of tests.

@rcomer
Copy link
Member Author

rcomer commented Jan 26, 2023

I looked at the py38 CI on another recent PR, and it has proj-8, but this is getting proj-7.2.

@greglucas
Copy link
Contributor

I thought proj was only being used for geodesics at this point, but perhaps that is the issue... I'm still hopeful that we'll rely exclusively on pyproj in the future so we don't have separate pins for the two.

proj8 was released in March 2021, so we could probably pin that high if need be until we get the proj dependence removed: https://github.com/OSGeo/PROJ/releases

It also looks like we need to update something with the sphinx gallery logo for the docs too. Lets address those in separate PRs though.

@greglucas
Copy link
Contributor

The failling tests and docs are unrelated and have been addressed in other PRs.

@greglucas greglucas merged commit e0b0d1f into SciTools:main Jan 29, 2023
@rcomer
Copy link
Member Author

rcomer commented Jan 29, 2023

Thanks @greglucas!

@rcomer rcomer deleted the contour-extents branch February 9, 2023 11:41
@QuLogic QuLogic added this to the 0.21.2 milestone Feb 21, 2023
@greglucas greglucas modified the milestones: 0.21.2, 0.22 Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange extent behaviour when latitude value too high
3 participants