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 up the satellite projections (Fixes #1144) #1189

Merged
merged 2 commits into from
Nov 16, 2018

Conversation

dopplershift
Copy link
Contributor

@dopplershift dopplershift commented Nov 15, 2018

This refactors the Geostationary and NearsidePerspective projections so that they can individually set up their boundary, limits, etc. Doing so allows fixing the incorrect boundary being set for NearsidePerspective. The two projections can't share the math for the boundary because they are fundamentally different coordinate systems.

lib/cartopy/crs.py Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved
@dopplershift
Copy link
Contributor Author

Before this PR:

before

After:

after

(See #1144 for sample code.)

@dopplershift dopplershift added this to the 0.17 milestone Nov 15, 2018
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Mostly cleanup since we're updating things.

lib/cartopy/crs.py Outdated Show resolved Hide resolved
lib/cartopy/crs.py Outdated Show resolved Hide resolved

if b != a or globe.ellipse is not None:
warnings.warn('The proj "nsper" projection does not handle '
'elliptical globes.')
Copy link
Member

Choose a reason for hiding this comment

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

So I've been thinking, maybe I should've moved this to a base class. Maybe change the default Globe creation based on an __init__ parameter, or private class attribute. Something for another PR, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think there's a few more things we could hoist out of the leaf nodes in the tree of projections.

Copy link
Member

Choose a reason for hiding this comment

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

Or more specifically - into a factory of Globe rather than the __init__ of a baseclass.

lib/cartopy/tests/crs/test_nearside_perspective.py Outdated Show resolved Hide resolved
lib/cartopy/tests/crs/test_nearside_perspective.py Outdated Show resolved Hide resolved
This refactors the Geostationary and NearSidePerspective projections so
that they can individually set up their boundary, limits, etc. Doing so
allows fixing the incorrect boundary being set for NearSidePerspective.
The two projections can't share the math for the boundary because they
are fundamentally different coordinate systems.
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.

LGTM

Since these are two different projections, they should not share as
much, so I've decoupled them a bit and updated some values.
@QuLogic QuLogic merged commit c8bdd00 into SciTools:master Nov 16, 2018
@dopplershift dopplershift deleted the fix-sat-projections branch November 16, 2018 19:37
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.

4 participants