Skip to content

Conversation

@bblay
Copy link
Contributor

@bblay bblay commented Sep 14, 2012

Copy link
Member

Choose a reason for hiding this comment

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

The way this has been designed, the ellipse is not propagated as is the case for the superclass. I think it needs to be.

@pelson
Copy link
Member

pelson commented Sep 20, 2012

@bblay : There is an outstanding action on you before this can be merged. (handle ellipse and datum better)

Copy link
Member

Choose a reason for hiding this comment

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

Seems strange that I should set the ellipse keyword, but it only kicks in if I provide proj params.
Worse, I don't think we should be exposing the user to the proj4_params - it is an implementation detail that we are using proj.4 under the hood.

Copy link
Member

Choose a reason for hiding this comment

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

Seems strange...

Indeed. It looks a bit like a square peg has been driven into a round hole. Supplying the proj4_params argument completely circumvents the definition of the Geodetic class! One could now do: crs = Geodetic({'proj': 'tmerc', ...}) and end up with a "geodetic" CRS which is actually a transverse Mercator!

The way to avoid this would be to allow only the parameters relevant to the rotated geodetic, e.g.:

class Geodetic
    def __init__(self, pole_longitude, pole_latitude, ellipse, datum)

But now the Geodetic class looks just like the RotatedGeodetic class! Which highlights the need for either:

  1. Make Geodetic a subclass of RotatedGeodetic.
  2. Get rid of the inheritance between Geodetic and RotatedGeodetic.

Out of those two I'd rather have option (2). It gives us what we need with the minimum of complexity.

@pelson
Copy link
Member

pelson commented Sep 20, 2012

Obviously, I have some concerns about the implementation details with regards to this PR, but I think this is a great new feature and I am very eager to merge! Thanks fro your work @bblay!

@bblay
Copy link
Contributor Author

bblay commented Sep 21, 2012

Thanks, doing these actions.

Why are Geodetic and Geocentric in _crs.pyx rather than crs.py?
I suppose we'd prefer RotatedGeodetic to live in the same place as those classes?

@rhattersley
Copy link
Member

Why are Geodetic and Geocentric in _crs.pyx rather than crs.py?
I suppose we'd prefer RotatedGeodetic to live in the same place as those classes?

I don't think there's a reason they have to be in _crs.pyx, I think it's just because they're so fundamental and because they're used (slightly naughtily, one might argue) within the CRS class. The RotatedGeodetic class isn't fundamental, so I've no problem with it living in crs.py.

Copy link
Member

Choose a reason for hiding this comment

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

And this one.

rhattersley added a commit that referenced this pull request Sep 21, 2012
Support for rotated geodetic CRS.
@rhattersley rhattersley merged commit a541c1f into SciTools:master Sep 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants