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

improve polygon coordinate handling #506

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

iamdonovan
Copy link

@iamdonovan iamdonovan commented Apr 1, 2024

As detailed/explained/requested in #504:

  • Added support for shapely geometry, WKT strings, and WKB geometries
  • checks that polygon coordinates are correctly oriented before passing to CMR
  • added shapely>=2.0.0 as a dependency to pyproject.toml

📚 Documentation preview 📚: https://earthaccess--506.org.readthedocs.build/en/506/

def polygon(
self,
coordinates: Union[
shapely.geometry.polygon.Polygon, str, bytes, List[Tuple[str, str]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
shapely.geometry.polygon.Polygon, str, bytes, List[Tuple[str, str]]
shapely.geometry.base.BaseGeometry, str, bytes, List[Tuple[str, str]]

I think since the rest of the implementation works of off the BaseGeometry, we could just use that here as well. We probably want to add some tests with different geometries and especially a clockwise vs couterclockwise ones

Copy link
Author

Choose a reason for hiding this comment

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

Done (583de40). Will think about/work on tests later this week/next week.


if isinstance(coordinates, shapely.geometry.base.BaseGeometry):
geom = coordinates
elif isinstance(coordinates, list):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using list is too restrictive. This should likely be Sequence.

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