Skip to content

Conversation

@gadomski
Copy link
Member

@gadomski gadomski commented Apr 10, 2023

Related Issue(s):

Description:
Items whose assets include data over the poles can be hard to represent in WGS84, which is required for their geometries. This PR adds a new function, stactools.core.utils.antimeridian.enclose_poles, that modifies geometries that cross the antimerdian to enclose the polar regions.

Explanation

Here's some sentinel5p data:

image

As you can see, the data encloses both the north and the south poles. A simple geometry will follow the boundary of the data, but doesn't display (or really make sense at all) on a map:

Screenshot 2023-04-10 at 7 22 43 AM

To solve the problem, we can "box" the poles by detecting antimeridian crossings and adding points to cover the polar regions in WGS84:

Screenshot 2023-04-10 at 7 23 38 AM

This last image shows a geometry created with enclose_poles.

PR checklist:

  • Code is formatted (run scripts/format).
  • Code lints properly (run scripts/lint).
  • Tests pass (run scripts/test).
  • Documentation has been updated to reflect changes, if applicable.
  • Changes are added to the CHANGELOG.

@gadomski gadomski requested a review from TomAugspurger April 10, 2023 13:25
@gadomski gadomski self-assigned this Apr 10, 2023
Copy link
Collaborator

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

One request for an additional test, which I think is important.

Only other question would be whether we want a test that verifies we raise a ValueError when the polygon has an interior ring. I'm not 100% sure why that matters (and am OK remaining ignorant here), so I'll let you judge whether a test for that case is important.

@gadomski
Copy link
Member Author

Only other question would be whether we want a test that verifies we raise a ValueError when the polygon has an interior ring. I'm not 100% sure why that matters (and am OK remaining ignorant here), so I'll let you judge whether a test for that case is important.

I don't think its particularly necessary, as this is more of a one-off utility rather than a foundational API function. I'm sure there's plenty of other edge cases that would blow this function up that aren't tested.

That being said, I have a thought about one more edge case we could test for -- the case where there's more than two antimeridian crossings (e.g. a swath over the Pacific). I'm going to convert to draft until I can cook up a test case (or better yet, a real-world example) of that and see if we can handle it.

@gadomski gadomski marked this pull request as draft April 10, 2023 14:14
@gadomski gadomski marked this pull request as ready for review April 10, 2023 16:32
@gadomski
Copy link
Member Author

@TomAugspurger I've added code and tests (and comments, because it's reasonably complex code) to handle multiple crossings. Ready for re-review, thanks.

Copy link
Collaborator

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@gadomski gadomski enabled auto-merge (rebase) April 10, 2023 16:39
@gadomski gadomski added this to the 0.4.6 milestone Apr 10, 2023
@gadomski gadomski merged commit 78a611a into main Apr 10, 2023
@gadomski gadomski deleted the enclose-poles branch April 10, 2023 16:48
@jlaura
Copy link

jlaura commented Apr 10, 2023

Just a quick kudos post @gadomski. This functionality works wonderfully on some Mars polar data!

Before:
Screen Shot 2023-04-10 at 2 20 14 PM

After:
Screen Shot 2023-04-10 at 2 20 22 PM

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.

4 participants