Fix stereographic scaling issues #929

dopplershift · 2017-09-01T19:08:04Z

This picks back up #456 and fixes #339 and #455

added scale_factor to Stereographic projection.
warnings when true_scale_latitude is used with non-polar
stereographic projections
test_stereographic extended

cc @QuLogic since you reviewed this previously

pelson

This looks good to me. Probably needs a rebase to get the tests passing. Otherwise, 👍

pelson · 2018-01-11T11:30:16Z

@dopplershift - no doubt you're running around like crazy with AMS, but if you get a chance, I think this just needs a rebase.

Fixes SciTools#339 and SciTools#455 - added scale_factor to Stereographic projection. - warnings when true_scale_latitude is used with non-polar stereographic projections - test_stereographic extended

stickler-ci · 2018-01-12T00:23:16Z

lib/cartopy/crs.py


 class NorthPolarStereo(Stereographic):
-    def __init__(self, central_longitude=0.0, globe=None):
+    def __init__(self, central_longitude=0.0, true_scale_latitude=None, globe=None):


E501 line too long (84 > 79 characters)

stickler-ci · 2018-01-12T00:23:16Z

lib/cartopy/crs.py

            central_latitude=90,
-            central_longitude=central_longitude, globe=globe)
+            central_longitude=central_longitude,
+            true_scale_latitude=true_scale_latitude, # None is equivalent to +90


E261 at least two spaces before inline comment
E501 line too long (80 > 79 characters)

stickler-ci · 2018-01-12T00:23:16Z

lib/cartopy/crs.py


 class SouthPolarStereo(Stereographic):
-    def __init__(self, central_longitude=0.0, globe=None):
+    def __init__(self, central_longitude=0.0, true_scale_latitude=None, globe=None):


E501 line too long (84 > 79 characters)

stickler-ci · 2018-01-12T00:23:17Z

lib/cartopy/crs.py

            central_latitude=-90,
-            central_longitude=central_longitude, globe=globe)
+            central_longitude=central_longitude,
+            true_scale_latitude=true_scale_latitude, # None is equivalent to -90


E261 at least two spaces before inline comment
E501 line too long (80 > 79 characters)

dopplershift · 2018-01-12T00:23:49Z

Done.

Yeah, AMS was pretty crazy--but amazing.

stickler-ci · 2018-01-12T00:37:43Z

lib/cartopy/crs.py

            central_latitude=90,
-            central_longitude=central_longitude, globe=globe)
+            central_longitude=central_longitude,
+            true_scale_latitude=true_scale_latitude, # None is +90


E261 at least two spaces before inline comment

stickler-ci · 2018-01-12T00:37:44Z

lib/cartopy/crs.py

            central_latitude=-90,
-            central_longitude=central_longitude, globe=globe)
+            central_longitude=central_longitude,
+            true_scale_latitude=true_scale_latitude, # None is -90


E261 at least two spaces before inline comment

QuLogic · 2018-01-12T03:43:17Z

lib/cartopy/crs.py

                        ('lon_0', central_longitude),
                        ('x_0', false_easting), ('y_0', false_northing)]
+
        if true_scale_latitude:


It's old, but should this be is not None? Since the *PolarStereo comments say defaults are not 0, it seems like you might want to pass a true scale latitude of 0, and that would be ignored with the code like this. Or is that basically useless since 0 is at the border?

Of course you find that after I think I'm done. 😁 Checking for None makes sense to me since the latitude is float anyway--checking just for 0 is worthless.

QuLogic · 2018-01-12T03:44:10Z

@pelson How are you going to proceed when the original author never submitted a CLA?

pelson · 2018-01-12T12:24:38Z

#456 (comment) suggests this was resolved. This is also reflected in https://github.com/SciTools/scitools.org.uk/blob/gh-pages/contributors.json#L36. I'll need to do some digging to figure out why the CLA check is failing.

dopplershift · 2018-01-16T21:13:19Z

Alright, finally green (minus CLA).

pelson

I've suggested a few tweaks. But am happy in general with this. thanks @dopplershift 👍

pelson · 2018-01-17T11:04:38Z

lib/cartopy/crs.py

+            if true_scale_latitude is not None:
+                warnings.warn('It does not make sense to provide both '
+                              '"scale_factor" and "true_scale_latitude". '
+                              'Ignoring "scale_factor".')


I've just realised that this is new behaviour - I'd support this being an exception, not a warning.

Makes sense.

pelson · 2018-01-17T11:05:32Z

lib/cartopy/crs.py

+                              'the use of "scale_factor" instead.')
            proj4_params.append(('lat_ts', true_scale_latitude))
+
+        if scale_factor:


I'd be more comfortable if we checked None explicitly. (I know scale_factor=0 doesn't really make any sense, but I'd rather not ignore it silently.)

QuLogic

Assuming @pelson can figure out the CLA.

QuLogic · 2018-01-26T04:53:46Z

Ping @pelson? Last thing for 0.16...

ajdawson · 2018-01-26T16:26:31Z

Just looks like the ID used to make the commit is not the same as the Github username. Can either get the original author to make the association to please the CI tool, or just take it that this commit obviously came from that person who we have a CLA for.

QuLogic · 2018-02-02T21:40:04Z

Well, I'm merging this then.

pelson added Component: proj SemVer: minor Type: Bug labels Nov 20, 2017

pelson self-requested a review November 20, 2017 12:19

pelson approved these changes Jan 3, 2018

View reviewed changes

Fix issues with scaling and stereographic projections

dab79a6

Fixes SciTools#339 and SciTools#455 - added scale_factor to Stereographic projection. - warnings when true_scale_latitude is used with non-polar stereographic projections - test_stereographic extended

dopplershift force-pushed the fix_issues_339_455 branch from 4e8253e to 9e4821e Compare January 12, 2018 00:22

stickler-ci reviewed Jan 12, 2018

View reviewed changes

dopplershift force-pushed the fix_issues_339_455 branch from 9e4821e to 1af34d7 Compare January 12, 2018 00:37

stickler-ci reviewed Jan 12, 2018

View reviewed changes

dopplershift force-pushed the fix_issues_339_455 branch from 1af34d7 to dbdf9b2 Compare January 12, 2018 00:40

QuLogic reviewed Jan 12, 2018

View reviewed changes

dopplershift force-pushed the fix_issues_339_455 branch 2 times, most recently from 1e282c4 to 1405f2e Compare January 15, 2018 19:49

pelson reviewed Jan 17, 2018

View reviewed changes

Address comments from review

da433a5

dopplershift force-pushed the fix_issues_339_455 branch from 1405f2e to da433a5 Compare January 18, 2018 00:18

dopplershift added this to the 0.16 milestone Jan 18, 2018

QuLogic approved these changes Jan 18, 2018

View reviewed changes

QuLogic merged commit 6f56b97 into SciTools:master Feb 2, 2018

dopplershift deleted the fix_issues_339_455 branch February 26, 2018 23:17

pp-mo mentioned this pull request Mar 9, 2018

Reorder Stereographic args to avoid API breaking 0.15 -> 0.16. #1050

Closed

Fix stereographic scaling issues #929

Fix stereographic scaling issues #929

Uh oh!

Conversation

dopplershift commented Sep 1, 2017

Uh oh!

pelson left a comment

Choose a reason for hiding this comment

Uh oh!

pelson commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dopplershift commented Jan 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dopplershift Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jan 12, 2018

Uh oh!

pelson commented Jan 12, 2018

Uh oh!

dopplershift commented Jan 16, 2018

Uh oh!

pelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jan 26, 2018

Uh oh!

ajdawson commented Jan 26, 2018

Uh oh!

QuLogic commented Feb 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dopplershift Jan 13, 2018 •

edited

Loading