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 antimeridian helpers to utils #259

Merged
merged 6 commits into from
Mar 24, 2022
Merged

Add antimeridian helpers to utils #259

merged 6 commits into from
Mar 24, 2022

Conversation

gadomski
Copy link
Member

@gadomski gadomski commented Mar 15, 2022

Related Issue(s): n/a

Description:
Includes some new helper functions:

  • normalize: returns a new Polygon that has all of its longitudes in the same sign, even if this means going beyond -180 or 180.
  • split: returns a new MultiPolygon that contains polygons on either side of the antimeridian, per the GeoJSON recommendations.
  • fix_item: applies these change to an Item's geometry, inplace

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.

Copy link
Collaborator

@pjhartzell pjhartzell 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. There is an edge case where it does not work as expected: polygons enclosing the north or south pole. Perhaps add that caveat in the docstring and put that down as a TODO.

@gadomski
Copy link
Member Author

@pjhartzell done in f31524f.

@gadomski gadomski requested a review from mmcfarland March 16, 2022 18:09
@gadomski gadomski marked this pull request as draft March 18, 2022 17:55
@gadomski
Copy link
Member Author

Converting to draft until I implement the functionality described in stactools-packages/landsat#13 (comment).

@gadomski gadomski marked this pull request as ready for review March 22, 2022 22:41
@gadomski
Copy link
Member Author

@pjhartzell I've added fix_item if you want to take a look.

Includes two helpers:
- `normalize`: returns a new Polygon that has all of its longitudes in the same
   sign, even if this means going beyond -180 or 180.
- `split`: returns a new MultiPolygon that contains polygons on either side of
  the antimeridian, per the GeoJSON recommendations.
@lossyrob
Copy link
Member

I was expecting the behavior to be that if a geometry was more "westernly" than "easternly", then the normalization would use negative numbers instead of positive. E.g.

def test_item_fix_antimeridian_normalize_west(self) -> None:
        canonical = Polygon(
            ((170, 40), (170, 50), (-140, 50), (-140, 40), (170, 40)))
        item = Item("an-id",
                    geometry=shapely.geometry.mapping(canonical),
                    bbox=canonical.bounds,
                    datetime=datetime.datetime.now(),
                    properties={})
        antimeridian.fix_item(item, antimeridian.Strategy.NORMALIZE)
        expected = shapely.geometry.box(170, 40, 190, 50)
        assert shapely.geometry.shape(item.geometry).equals(expected)
        assert item.bbox
        assert list(item.bbox) == [-190.0, 40.0, -140.0, 50.0]

This would allow e.g. STAC queries that are asking for western areas to pick up the appropriate geometries. As it stands it looks like anything that crosses the antimeridian from the west will get labeled as a very east polygon.

@gadomski
Copy link
Member Author

@lossyrob done in d8b9d41.

Copy link
Member

@lossyrob lossyrob left a comment

Choose a reason for hiding this comment

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

Couple of small suggestions, but besides the known TODO this LGTM

src/stactools/core/utils/antimeridian.py Show resolved Hide resolved
tests/core/test_utils.py Outdated Show resolved Hide resolved
@gadomski gadomski merged commit c7de095 into main Mar 24, 2022
@gadomski gadomski deleted the antimeridian branch March 24, 2022 12:53
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