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

Use boundary to compute axes domain in gridliner #1538

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stefraynaud
Copy link
Contributor

@stefraynaud stefraynaud commented Apr 27, 2020

Rationale

Gridliner._axes_domain discretizes the unitary domain to find min/max of longitudes and latitudes.
This discretization makes approximative the estimate if bounds.

This PR directly uses native projection boundary to find min and max.
The PR is quite simple and I may have missed some subtleties.

I didn't updated the figures since alot of them are modified and I'm waiting for feedback.

Implications

Gridlines are properly plotted until the boundary.

Fixes #1443

lonlat_range

@stefraynaud
Copy link
Contributor Author

I think this approach cannot be used alone, since it will never work for polar plot for example.
Instead, it but to used to make the base algo more robust.

@stefraynaud
Copy link
Contributor Author

Now it works well with the best of the two algo.

@dopplershift
Copy link
Contributor

Could you add the image as a test?

lib/cartopy/mpl/gridliner.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_gridliner.py Outdated Show resolved Hide resolved
@QuLogic
Copy link
Member

QuLogic commented Aug 20, 2020

This may need a rebase to fix CI.

@stefraynaud stefraynaud changed the base branch from master to v0.18.x October 13, 2020 13:37
@stefraynaud stefraynaud changed the base branch from v0.18.x to master October 13, 2020 13:49
@greglucas
Copy link
Contributor

It looks like you've brought some duplicate commits into this. Can you try to rebase onto master instead of merging master into this branch? I'm not sure why Appveyor failed on the gridliner image tests, but Travis didn't. Maybe it will fix itself with a rerun of the CI...

@stefraynaud
Copy link
Contributor Author

@greglucas how to proceed with this rebasing?

@greglucas
Copy link
Contributor

I think you have to delete the commits through an interactive rebase, or at least that's how I've done it when things get out of sync between branches.

  1. checkout master and fetch/pull the latest master commits from upstream
  2. checkout this branch
  3. git rebase -i master

That should give you a list of all the commits to choose from and then just keep the ones you want and delete the others. Probably just keeping all of your commits and removing all the merges/other people's commits from the list.

Fix SciTools#1443

Merge the old and the new algo for the axes domain estimate

The coordinates of the outline are simply added to the rasterised coordinates.

In addition, the rasterized coordinates are generated with a non-even number of ticks to make sure to catch the center when bounds are symetric.

Add gridliner_orthographic test

This tests Gridliner._axes_domain

Update figs due to new gridliner axes domain

Fix figure testing for crs

Fix tolerance for test_gridliner

Remove unneeded call to ravel
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

Gridlines not plotted correctly with Mollweide and Orthographic projections
5 participants