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

Many known geometry transforms fixed. #545

Merged
merged 4 commits into from Feb 13, 2015

Conversation

Projects
None yet
4 participants
@pelson
Member

pelson commented Feb 11, 2015

Closes #45, #259 (both cases within) and #541.

Does not close: #77 (adding a set_global shows that the transform isn't perfect), #325

Need to rationalise (i.e. reduce to single cases): #318.

A little further work will render #492 fixable (already it is better, as it stops the globe being filled incorrectly, but Antarctica is being filled).

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Feb 11, 2015

Member

As a follow-up PR, I intend to move most of this code out of crs.py.

Member

pelson commented Feb 11, 2015

As a follow-up PR, I intend to move most of this code out of crs.py.

@pelson pelson added this to the 0.12 milestone Feb 11, 2015

@@ -338,43 +339,98 @@ def boundary_distance(xy):
# Record the positions of all the segment ends
for i, line_string in enumerate(line_strings):
first_dist = boundary_distance(line_string.coords[0])
thing = _Thing(first_dist, False,
(i, 'first', line_string.coords[0]))
thing = _BoundaryPoint(first_dist, False,

This comment has been minimized.

@rhattersley

rhattersley Feb 11, 2015

Member

👍 Oh boy, has that been a long time coming!

@rhattersley

rhattersley Feb 11, 2015

Member

👍 Oh boy, has that been a long time coming!

This comment has been minimized.

@rhattersley

rhattersley Feb 11, 2015

Member

Although I guess it might be nice to rename the thing variables as well.

@rhattersley

rhattersley Feb 11, 2015

Member

Although I guess it might be nice to rename the thing variables as well.

This comment has been minimized.

@pelson

pelson Feb 11, 2015

Member

Refactor needed - I'm just going to lift the whole lot out of crs.py. And yes, there will be no mention of "thing" anywhere 😉 .

@pelson

pelson Feb 11, 2015

Member

Refactor needed - I'm just going to lift the whole lot out of crs.py. And yes, there will be no mention of "thing" anywhere 😉 .

Show outdated Hide outdated lib/cartopy/crs.py
# tiny fraction.
last_dist += 1e-8 # TODO: Use epsilon.
thing = _BoundaryPoint(last_dist, False,
(i, 'self-closing',

This comment has been minimized.

@pelson

pelson Feb 11, 2015

Member

It seems this concept isn't needed now I've dealt with co-incident _BoundaryPoint sections.

@pelson

pelson Feb 11, 2015

Member

It seems this concept isn't needed now I've dealt with co-incident _BoundaryPoint sections.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Feb 11, 2015

Member

I'm tempted to bank this, and look at any remaining issues separately. As this is a slightly more involved change would you be able to take a look at some point @rhattersley? If not in the short term, I'll continue working on this PR until perhaps Friday afternoon/Monday.

Cheers,

Member

pelson commented Feb 11, 2015

I'm tempted to bank this, and look at any remaining issues separately. As this is a slightly more involved change would you be able to take a look at some point @rhattersley? If not in the short term, I'll continue working on this PR until perhaps Friday afternoon/Monday.

Cheers,

@esc24

This comment has been minimized.

Show comment
Hide comment
@esc24

esc24 Feb 12, 2015

Member

Just looking a the visual result, this clearly improves things, but it's interesting that the top and bottom of the IGH boundary are flattened.

Member

esc24 commented Feb 12, 2015

Just looking a the visual result, this clearly improves things, but it's interesting that the top and bottom of the IGH boundary are flattened.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Feb 12, 2015

Member

Just looking a the visual result, this clearly improves things, but it's interesting that the top and bottom of the IGH boundary are flattened.

Yeah, agreed. Curious, but not outright worrying.

Member

pelson commented Feb 12, 2015

Just looking a the visual result, this clearly improves things, but it's interesting that the top and bottom of the IGH boundary are flattened.

Yeah, agreed. Curious, but not outright worrying.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Feb 13, 2015

Member

Having spoken to @rhattersley (who is the only person who with the kind of knowledge of the algorithm needed to review this algorithmically) I think this should be merged. If there are any general (i.e. non-algorithmic) improvements that can be suggested based on this PR, I'd be happy to address them in a follow-up PR.

Member

pelson commented Feb 13, 2015

Having spoken to @rhattersley (who is the only person who with the kind of knowledge of the algorithm needed to review this algorithmically) I think this should be merged. If there are any general (i.e. non-algorithmic) improvements that can be suggested based on this PR, I'd be happy to address them in a follow-up PR.

pelson added a commit that referenced this pull request Feb 13, 2015

@pelson pelson merged commit 715bad7 into SciTools:master Feb 13, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

@pelson pelson deleted the pelson:all_known_geometry_transforms_fixed branch Feb 13, 2015

@QuLogic QuLogic modified the milestones: 0.12, v0.12.0 Mar 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment