-
Notifications
You must be signed in to change notification settings - Fork 94
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
[Future Spherical Class] Add SExtent #466
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #466 +/- ##
==========================================
+ Coverage 94.27% 94.29% +0.02%
==========================================
Files 69 74 +5
Lines 12394 12929 +535
==========================================
+ Hits 11685 12192 +507
- Misses 709 737 +28
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically this is to handle all the edge cases of extents that wouldn't make sense to include in a generic Polygon class, right?
sext = SExtent([extent, extent]) | ||
sext = SExtent(extent, extent, ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need or benefit to supporting both of these types of multi-extent inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need. Which one would you suggest to drop?
I would keep the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it's been a long time since I've looked at this and I can't quite remember what this class does. So it merges extents (bounding box?) into a single MultiPolygon? The idea being you either have one contiguous extent or you have two extents that "cross" the antimeridian, right? Will there ever be a need for more than two extents?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. One could discretize a SPolygon in lot of non-intersecting extents ...
But the main use case is to deal with the antimeridian ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what I prefer. Initially I was going to say only use the second one (passing a list), but since the usual case is a single 4-element extent then maybe the first and the third should be the only ways to do it.
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
This PR adds the SExtent class of the future spherical geometry interface.
git diff origin/main **/*py | flake8 --diff