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

Add more options to Mercator projection #1043

Merged
merged 1 commit into from
Jul 15, 2018

Conversation

dopplershift
Copy link
Contributor

@dopplershift dopplershift commented Feb 28, 2018

Closes #1040.

This adds scale_factor as an additional way to control the scaling (in addition to latitude_true_scale). It also adds support for false_easting and false_northing. These options are all required to be fully capable of representing Mercator projection coming from a CF-compliant netCDF file.

def test_easting_northing():
false_easting = 1000000
false_northing = -2000000
crs = ccrs.Mercator(false_easting=false_easting, false_northing=false_northing)

Choose a reason for hiding this comment

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

E501 line too long (83 > 79 characters)

false_northing = -2000000
crs = ccrs.Mercator(false_easting=false_easting, false_northing=false_northing)
proj4_str = ('+ellps=WGS84 +proj=merc +lon_0=0.0 +x_0={} +y_0={} '
'+units=m +lat_ts=0.0 +no_defs'.format(false_easting, false_northing))

Choose a reason for hiding this comment

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

E501 line too long (87 > 79 characters)



def test_scale_factor():
scale_factor = 0.9396926207859084 # Should be same as lat_ts=20 for a sphere

Choose a reason for hiding this comment

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

E501 line too long (81 > 79 characters)


def test_scale_factor():
scale_factor = 0.9396926207859084 # Should be same as lat_ts=20 for a sphere
crs = ccrs.Mercator(scale_factor=scale_factor, globe=ccrs.Globe(ellipse='sphere'))

Choose a reason for hiding this comment

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

E501 line too long (86 > 79 characters)

('units', 'm'),
('lat_ts', latitude_true_scale)]
if scale_factor is not None:
if latitude_true_scale:
Copy link
Member

Choose a reason for hiding this comment

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

You'll have to make the default None now; you can't assume the user didn't pass scale_factor and latitude_true_scale=0 explicitly expecting it to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sigh Yeah...

crs = ccrs.Mercator(scale_factor=scale_factor,
globe=ccrs.Globe(ellipse='sphere'))
proj4_str = ('+ellps=sphere +proj=merc +lon_0=0.0 +x_0=0.0 +y_0=0.0 '
'+units=m +k_0={} +no_defs'.format(scale_factor))
Copy link
Member

Choose a reason for hiding this comment

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

scale_factor is not formatting consistently on Py 2/3. Might be simpler to just hard-code the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@dopplershift
Copy link
Contributor Author

Not sure why circleCI is failing.

('units', 'm')]

# If it's None, we use the default for Proj4, which is 0.0
Copy link
Member

Choose a reason for hiding this comment

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

The comment reads as if we are going to pass 0.0, but I guess you mean we implicitly use 0.0 because that is what proj4 assumes when we don't use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. I've tried to clarify that comment.

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.

The joys of mutually exclusive keywords. 😢 (proj's doing, not yours)

I've generally tried to avoid them wherever possible, but I recognise that sometimes one wants to setup their projection based on what they have, and not have to convert it in some way.

Unfortunately, introducing mutually exclusive keywords makes things a real pain for testing equivalence - I believe that is something we have to address (perhaps not in this PR, but generally within cartopy), otherwise the cartopy Projection classes become (a pretty second-rate) direct wrapper on top of proj.

Have you looked into how easy it would be to normalize one into the other by any chance? If that were easy & reliable then that would be my preferred approach to solving this issue (I'd still happily have mutually exclusive keywords, but would instead normalize them in the init).

This adds scale_factor as an additional way to control the scaling (in
addition to latitude_true_scale). It also adds support for
false_easting and false_northing. These options are all required to be
fully capable of representing Mercator projection coming from a
CF-compliant netCDF file.
proj4_str = ('+ellps=WGS84 +proj=merc +lon_0=0.0 +lat_ts={} '
'+units=m +no_defs'.format(lat_ts))
proj4_str = ('+ellps=WGS84 +proj=merc +lon_0=0.0 +x_0=0.0 +y_0=0.0 '
'+units=m +lat_ts={} +no_defs'.format(lat_ts))
Copy link
Member

Choose a reason for hiding this comment

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

Woopsy..... 👎

Copy link
Member

Choose a reason for hiding this comment

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

Oh, weird. I don't actually know what is going on here... 😕

@pelson pelson merged commit e550c47 into SciTools:master Jul 15, 2018
@dopplershift dopplershift deleted the mercator-options branch July 15, 2018 21:49
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.

None yet

5 participants