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

Added additonal args to TransverseMercator #309

Merged
merged 3 commits into from Aug 21, 2013

Conversation

Projects
None yet
3 participants
@esc24
Member

esc24 commented Aug 2, 2013

This PR adds a number of additional arguments to the constructor of TransverseMercator. I have done this because I wish to plot data expressed in a TransverseMercator coordinate system with non-zero false easting and northing values. As cartopy currently stands this Transverse Mercator cannot be represented in Cartopy using the TransverseMercator class. This PR also modifies OSGB and OSNI so that they are now subclasses of TransverseMercator. I'd also like to make EuroPP a subclass of TransverseMercator but there are complications over adding the zone parameter so I have left that for the future.

Note that I am unsure of the appropriate values for the limits of the projection (previously they were +-180 and +-90 pseudo-degrees based on a unit earth radius) so I have chosen values that result in a similar looking global plot with the default parameter values. This removal of an artificial unit radius means a change in the xytick labels - see the test image change.

@bblay

This comment has been minimized.

Show comment
Hide comment
@bblay

bblay Aug 5, 2013

Contributor

I have chosen values that result in a similar looking global plot with the default parameter values

We're not seeing a global image change in the diffs, does this mean there wasn't one to start with?
If so, would you be able to pop in a "before and after" here please?
Also, please consider adding a global image test so further changes can be detected.

Contributor

bblay commented Aug 5, 2013

I have chosen values that result in a similar looking global plot with the default parameter values

We're not seeing a global image change in the diffs, does this mean there wasn't one to start with?
If so, would you be able to pop in a "before and after" here please?
Also, please consider adding a global image test so further changes can be detected.

@esc24

This comment has been minimized.

Show comment
Hide comment
@esc24

esc24 Aug 6, 2013

Member

We're not seeing a global image change in the diffs, does this mean there wasn't one to start with?

There is a test (cartopy.tests.mpl.test_mpl_integration.test_multiple_projections) that does a global plot and two curves that covers the TransverseMercator and other projections. There is no discernible difference (but the plots are fairly small).

Also, please consider adding a global image test so further changes can be detected.

I'm also inclined to add a local test e.g. TransverseMercator with OSGB values to give more coverage of the other parameters like your Lambert Australia test . What do you think?

Member

esc24 commented Aug 6, 2013

We're not seeing a global image change in the diffs, does this mean there wasn't one to start with?

There is a test (cartopy.tests.mpl.test_mpl_integration.test_multiple_projections) that does a global plot and two curves that covers the TransverseMercator and other projections. There is no discernible difference (but the plots are fairly small).

Also, please consider adding a global image test so further changes can be detected.

I'm also inclined to add a local test e.g. TransverseMercator with OSGB values to give more coverage of the other parameters like your Lambert Australia test . What do you think?

@bblay

This comment has been minimized.

Show comment
Hide comment
@bblay

bblay Aug 6, 2013

Contributor

Good thinking.

Contributor

bblay commented Aug 6, 2013

Good thinking.

@property
def threshold(self):
return 0.5

This comment has been minimized.

@pelson

pelson Aug 12, 2013

Member

How come this was removed? Is this the inherited value?

@pelson

pelson Aug 12, 2013

Member

How come this was removed? Is this the inherited value?

This comment has been minimized.

@esc24

esc24 Aug 12, 2013

Member

I haven't removed it (see https://github.com/esc24/cartopy/blob/26b42236feb556fb84f346147c4d547561db28bd/lib/cartopy/crs.py#L671-L673) but I've changed the value as I've taken out the dubious 'make metres ~= degrees' Earth radius.

@esc24

esc24 Aug 12, 2013

Member

I haven't removed it (see https://github.com/esc24/cartopy/blob/26b42236feb556fb84f346147c4d547561db28bd/lib/cartopy/crs.py#L671-L673) but I've changed the value as I've taken out the dubious 'make metres ~= degrees' Earth radius.

@property
def x_limits(self):
return (-2e7, 2e7)

This comment has been minimized.

@pelson

pelson Aug 12, 2013

Member

Just wondering if this is the universal limit of the Proj.4 TansverseMercator, or whether there are parameters you can change which influence this value?

@pelson

pelson Aug 12, 2013

Member

Just wondering if this is the universal limit of the Proj.4 TansverseMercator, or whether there are parameters you can change which influence this value?

This comment has been minimized.

@esc24

esc24 Aug 12, 2013

Member

I suspect the values should change based on central lat/lon and false easting/northing. However I did some experimentation and couldn't come up with anything robust (I was trying to reproduce https://en.wikipedia.org/wiki/File:MercTranSph.png but failed). I'm hoping we can tackle this issue when a use case arises (e.g. plotting Austrialia), but I appreciate this is a significant issue.

@esc24

esc24 Aug 12, 2013

Member

I suspect the values should change based on central lat/lon and false easting/northing. However I did some experimentation and couldn't come up with anything robust (I was trying to reproduce https://en.wikipedia.org/wiki/File:MercTranSph.png but failed). I'm hoping we can tackle this issue when a use case arises (e.g. plotting Austrialia), but I appreciate this is a significant issue.

This comment has been minimized.

@pelson

pelson Aug 13, 2013

Member

Ah, ok. So I think this number is only influenced by the globe that you pass, not by any of the parameters such as lon0 etc.

In which case, I think we need to hard code the globe definition rather than claim general support without having it.

Would you agree with that?

@pelson

pelson Aug 13, 2013

Member

Ah, ok. So I think this number is only influenced by the globe that you pass, not by any of the parameters such as lon0 etc.

In which case, I think we need to hard code the globe definition rather than claim general support without having it.

Would you agree with that?

This comment has been minimized.

@esc24

esc24 Aug 13, 2013

Member

Would you agree with that?

I would not. In my view the current TransverseMercator is of almost no practical use because of its fixed parameters. I appreciate this is far from perfect, but it makes it usable. If we hard code the globe, the OSGB and OSNI cannot inherit from TransverseMercator (or only one can 😱) and we are almost back to where we were - i.e. I can't plot my data (it's on a tmerc grid with a Globe(datum='OSGB36', ellipse='airy') globe). Does your logic apply to the other projections that take a globe and have hard coded limits or is this a peculiarity of the TransverseMercator?

@esc24

esc24 Aug 13, 2013

Member

Would you agree with that?

I would not. In my view the current TransverseMercator is of almost no practical use because of its fixed parameters. I appreciate this is far from perfect, but it makes it usable. If we hard code the globe, the OSGB and OSNI cannot inherit from TransverseMercator (or only one can 😱) and we are almost back to where we were - i.e. I can't plot my data (it's on a tmerc grid with a Globe(datum='OSGB36', ellipse='airy') globe). Does your logic apply to the other projections that take a globe and have hard coded limits or is this a peculiarity of the TransverseMercator?

This comment has been minimized.

@pelson

pelson Aug 13, 2013

Member

Does your logic apply to the other projections that take a globe and have hard coded limits or is this a peculiarity of the TransverseMercator?

I have tried to apply that logic, yes, though I suspect that some have slipped through the net in the interest of keeping some momentum with particularly iterative PRs...

Ok, in the interest of keeping this particular PR moving, lets keep the variable globe option, and we can deal with any cases which do not fall into this particular case.

@pelson

pelson Aug 13, 2013

Member

Does your logic apply to the other projections that take a globe and have hard coded limits or is this a peculiarity of the TransverseMercator?

I have tried to apply that logic, yes, though I suspect that some have slipped through the net in the interest of keeping some momentum with particularly iterative PRs...

Ok, in the interest of keeping this particular PR moving, lets keep the variable globe option, and we can deal with any cases which do not fall into this particular case.

Show outdated Hide outdated lib/cartopy/crs.py
class OSGB(TransverseMercator):
def __init__(self):
globe = Globe(datum='OSGB36', ellipse='airy')

This comment has been minimized.

@pelson

pelson Aug 12, 2013

Member

Unused.

@pelson

pelson Aug 12, 2013

Member

Unused.

This comment has been minimized.

@esc24

esc24 Aug 12, 2013

Member

Yes.

@esc24

esc24 Aug 12, 2013

Member

Yes.

Show outdated Hide outdated lib/cartopy/mpl/geoaxes.py
@@ -571,7 +571,7 @@ def set_xticks(self, ticks, minor=False, crs=None):
"""
if not isinstance(self.projection, (ccrs._RectangularProjection,
ccrs._CylindricalProjection,
ccrs.OSGB)):
ccrs.TransverseMercator)):

This comment has been minimized.

@pelson

pelson Aug 12, 2013

Member

Not sure this is possible, I'm afraid. The code wont handle all TransverseMercators (but will handle limited ones such as OSGB and OSNI).

@pelson

pelson Aug 12, 2013

Member

Not sure this is possible, I'm afraid. The code wont handle all TransverseMercators (but will handle limited ones such as OSGB and OSNI).

This comment has been minimized.

@esc24

esc24 Aug 12, 2013

Member

Ok. I'll change this (and below).

@esc24

esc24 Aug 12, 2013

Member

Ok. I'll change this (and below).

Show outdated Hide outdated lib/cartopy/mpl/geoaxes.py
@@ -620,7 +620,7 @@ def set_yticks(self, ticks, minor=False, crs=None):
"""
if not isinstance(self.projection, (ccrs._RectangularProjection,
ccrs._CylindricalProjection,
ccrs.OSGB)):
ccrs.TransverseMercator)):

This comment has been minimized.

@pelson

pelson Aug 12, 2013

Member

Ditto.

@pelson

pelson Aug 12, 2013

Member

Ditto.

Show outdated Hide outdated lib/cartopy/tests/crs/test_transverse_mercator.py
import unittest
import matplotlib.pyplot as plt

This comment has been minimized.

@pelson

pelson Aug 12, 2013

Member

There is no image testing in the non mpl directory. Can I ask what these tests are for - they certainly feel like integration tests rather than anything specific. Is that fair?

@pelson

pelson Aug 12, 2013

Member

There is no image testing in the non mpl directory. Can I ask what these tests are for - they certainly feel like integration tests rather than anything specific. Is that fair?

This comment has been minimized.

@esc24

esc24 Aug 12, 2013

Member

I might not describe them exactly like that, but they aren't tightly focussed unit tests. I was basing this on the example of the recently merged LambertConformal PR #292 which has similar tests (see earlier review comments). They are aimed at testing the ability of cartopy to correctly project to TranverseMercator. I could have picked some points, but a geometry i.e. a coastline was a quick, thorough, albeit broad way to do it. The second aim is to test the parameters of the constructor i.e. passing the the parameters to the constructor does give the equivalent (except for limits) of an OSGB projection.

@esc24

esc24 Aug 12, 2013

Member

I might not describe them exactly like that, but they aren't tightly focussed unit tests. I was basing this on the example of the recently merged LambertConformal PR #292 which has similar tests (see earlier review comments). They are aimed at testing the ability of cartopy to correctly project to TranverseMercator. I could have picked some points, but a geometry i.e. a coastline was a quick, thorough, albeit broad way to do it. The second aim is to test the parameters of the constructor i.e. passing the the parameters to the constructor does give the equivalent (except for limits) of an OSGB projection.

This comment has been minimized.

@esc24

esc24 Aug 12, 2013

Member

FWIW I found the image testing in the crs directory a little surprising given our discussion around the feature API testing and putting it in the mpl directory, but I assumed you'd decided it was ok based on your merge of #292.

@esc24

esc24 Aug 12, 2013

Member

FWIW I found the image testing in the crs directory a little surprising given our discussion around the feature API testing and putting it in the mpl directory, but I assumed you'd decided it was ok based on your merge of #292.

This comment has been minimized.

@esc24

esc24 Aug 13, 2013

Member

@pelson - any thoughts on a suitable lightweight testing approach for CRSs? I appreciate the benefit of keeping a clear division between matplotlib related code and generic code, but I don't feel we need to be quite as rigid within testing.

@esc24

esc24 Aug 13, 2013

Member

@pelson - any thoughts on a suitable lightweight testing approach for CRSs? I appreciate the benefit of keeping a clear division between matplotlib related code and generic code, but I don't feel we need to be quite as rigid within testing.

This comment has been minimized.

@pelson

pelson Aug 13, 2013

Member

any thoughts on a suitable lightweight testing approach for CRSs?

Some known number transformations? Particularly if the addition of these Projections haven't resulted in needing any changes to the GeoAxes...

@pelson

pelson Aug 13, 2013

Member

any thoughts on a suitable lightweight testing approach for CRSs?

Some known number transformations? Particularly if the addition of these Projections haven't resulted in needing any changes to the GeoAxes...

This comment has been minimized.

@esc24

esc24 Aug 13, 2013

Member

Sounds good. Thanks.

@esc24

esc24 Aug 13, 2013

Member

Sounds good. Thanks.

Show outdated Hide outdated lib/cartopy/mpl/geoaxes.py
@@ -1048,7 +1048,8 @@ def _pcolormesh_patched(self, *args, **kwargs):
t = kwargs.get('transform', None)
if isinstance(t, ccrs.CRS):
wrap_proj_types = (ccrs._RectangularProjection,
ccrs._WarpedRectangularProjection)
ccrs._WarpedRectangularProjection,
ccrs.TransverseMercator)

This comment has been minimized.

@pelson

pelson Aug 13, 2013

Member

Just wondering which case this covers? I'm not sure that the code can actually handle TransverseMercator...

@pelson

pelson Aug 13, 2013

Member

Just wondering which case this covers? I'm not sure that the code can actually handle TransverseMercator...

This comment has been minimized.

@esc24

esc24 Aug 13, 2013

Member

I was attempting to maintain the current behaviour (as TransverseMercator previously inherited from ccrs._RectangularProjection). If I'm honest I'm not sure how this wrapping code works.

@esc24

esc24 Aug 13, 2013

Member

I was attempting to maintain the current behaviour (as TransverseMercator previously inherited from ccrs._RectangularProjection). If I'm honest I'm not sure how this wrapping code works.

@esc24

This comment has been minimized.

Show comment
Hide comment
@esc24

esc24 Aug 21, 2013

Member

@pelson - I know you're busy, but just a gentle reminder that I've made some changes following your comments. I'm growing more uneasy about the fixed threshold, but the same issue affects a number of the projections, so a generic solution might be needed.

Member

esc24 commented Aug 21, 2013

@pelson - I know you're busy, but just a gentle reminder that I've made some changes following your comments. I'm growing more uneasy about the fixed threshold, but the same issue affects a number of the projections, so a generic solution might be needed.

pelson added a commit that referenced this pull request Aug 21, 2013

Merge pull request #309 from esc24/tmerc_fix
Added additonal args to TransverseMercator

@pelson pelson merged commit ddc5a6e into SciTools:master Aug 21, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment