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

Improve warped rectangular projections #1180

Merged
merged 6 commits into from
Nov 14, 2018
Merged

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Nov 13, 2018

Improves warped rectangular projections in a few ways:

  • add the standard set of tests for Robinson
  • it turns out that Mollweide and Robinson do not support ellipses, so warn about them and remove tests for it
  • add support for offsets (false easting/northing) to Mollweide and Robinson
  • add docs for Mollweide and Robinson

@QuLogic
Copy link
Member Author

QuLogic commented Nov 13, 2018

Also, instead of using existing results as I did before, I worked out the limits from the equations (that's how I noticed Mollweide didn't support ellipses.)

Copy link
Member

@pelson pelson left a comment

Choose a reason for hiding this comment

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

Great stuff. Thanks @QuLogic.

lib/cartopy/crs.py Show resolved Hide resolved
lib/cartopy/crs.py Show resolved Hide resolved
@@ -175,9 +175,11 @@ def test_transform_points_xyz(self):
def test_globe(self):
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that this test hasn't changed based on the fact that the globe changed. Shows that the test is of questionable value. Perhaps the first thing to fix is the name of the variables (rugby_globe -> footy_globe and footy_globe -> tennisball_globe). And then we should think about whether this test needs to remain at all...

Thoughts?

Copy link
Member Author

@QuLogic QuLogic Nov 13, 2018

Choose a reason for hiding this comment

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

I mean, that's kind of the point; Mollweide doesn't support elliptical globes, so specifying ellipse or semiminor_axis does nothing.

This test doesn't actually check that the results are different in the end... only that they match some 'true' value, whatever that might be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, perhaps I was a bit unclear; that's the point of the changes in this file. As to the point of the test, I think it's just testing that the globe does something, though maybe it's become a bit redundant by the individual test_eccentric_ellipse tests in the crs directory.

Maybe we should add a test_globe for the ones that only support spheres and remove this test?

Copy link
Member

Choose a reason for hiding this comment

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

Yep. That works.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add a test_globe for the ones that only support spheres and remove this test?

Either that, or we keep the test but fix the variable name. Either works for me. I'll defer this though in order to move the PR forwards.

assert np.all(np.isnan(result))


def test_transform_points():
"""
Mostly tests the workaround for a specific problem.
Problem report in: https://github.com/SciTools/cartopy/issues/23
Copy link
Member

Choose a reason for hiding this comment

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

Missing a 2 at the end (issue 232, I believe)

@pelson pelson merged commit 88c2e3e into SciTools:master Nov 14, 2018
@pelson pelson added this to the 0.17 milestone Nov 14, 2018
@QuLogic QuLogic deleted the improve-warped branch November 14, 2018 04:55
@QuLogic
Copy link
Member Author

QuLogic commented Nov 15, 2018

Oh, I might have been wrong about Mollweide; it seems to make a difference using WGS84 spherical-ized semimajor radius vs. WGS84-the-ellipse. But I'm not sure exactly if it truly supports ellipses.

@QuLogic
Copy link
Member Author

QuLogic commented Nov 15, 2018

Oops, copy&paste type; was comparing against Robinson with the ellipse. Never mind.

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