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 enclose_poles to antimeridian module #416

Merged
merged 2 commits into from
Apr 10, 2023
Merged

Add enclose_poles to antimeridian module #416

merged 2 commits into from
Apr 10, 2023

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 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.

tests/core/utils/test_antimeridian.py Show resolved Hide resolved
@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.

3 participants