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

BUG: setting GeoAxes extent #4040

Merged
merged 5 commits into from
Aug 8, 2022

Conversation

chrishavlin
Copy link
Contributor

@chrishavlin chrishavlin commented Jul 29, 2022

This changes how the extent for GeoAxes is handled -- this version calls set_extent only if the data is not global and always passes the extent kwarg to imshow. Seemed to work well locally, but we'll see how CI does...

I may broaden the scope of this PR to include changing the default projection to set the central_longitude kwarg from the plot arguments when available, but if that proves to be too complex I'll just keep this PR to the extent modification. EDIT: Will look into this for a follow up PR.

chrishavlin and others added 2 commits August 3, 2022 12:28
code suggestion

Co-authored-by: Clément Robert <cr52@protonmail.com>
@chrishavlin chrishavlin marked this pull request as ready for review August 3, 2022 23:49
@chrishavlin
Copy link
Contributor Author

chrishavlin commented Aug 3, 2022

So in the latest batch of commits, among other things, I adjusted the test_geo_projections tests to use a linear instead of log scale for the colorbar scaling to avoid the matplotlib issue. I'm not entirely sure why it was failing now, but it was related to similar failures we've had with lognorm and certain data ranges. Since that should separately be fixed by #3849 and #3986, I just switched the tests here to use a linear scale. Could switch back at a later date, but I don't see a reason to not leave it using the linear scaling.

neutrinoceros
neutrinoceros previously approved these changes Aug 4, 2022
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Two quick comments, otherwise LGTM

yt/visualization/base_plot_types.py Outdated Show resolved Hide resolved
@chrishavlin
Copy link
Contributor Author

oops, just pushed to this branch accidentally... ignore the latest changes, will fix....

suggested geoaxes check

Co-authored-by: Clément Robert <cr52@protonmail.com>
@chrishavlin
Copy link
Contributor Author

ok. fixed the accidental push, updated the geoaxes check. and I want to add one more test then this will be good I think!

@neutrinoceros
Copy link
Member

I forgot to check that sys was already imported in my last suggestion. It wasn't.

@neutrinoceros
Copy link
Member

I fixed the 2 issues (one purely stylistic, reported by flake8) with my latest suggestion.
I'll review again after lunch when CI is complete

@neutrinoceros neutrinoceros self-requested a review August 5, 2022 10:12
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I think it's good to go. Before I merge, does this completely fix #4039 (which isn't currently linked) ?

@chrishavlin
Copy link
Contributor Author

Yes, this does fix the bug there. The other part of the discussion in #4039 (setting the central_longitude of the default projection when possible) is more of an enhancement that will take some extra work, I'll open up a new issue to track it.

@chrishavlin chrishavlin linked an issue Aug 8, 2022 that may be closed by this pull request
@neutrinoceros neutrinoceros merged commit fc0bf20 into yt-project:main Aug 8, 2022
neutrinoceros added a commit to neutrinoceros/yt that referenced this pull request Aug 8, 2022
@neutrinoceros neutrinoceros added this to the 4.0.5 milestone Aug 9, 2022
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.

Some geographic mapping issues
3 participants