benbovy / spherely

Manipulation and analysis of geometric objects on the sphere.
https://spherely.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
107 stars 6 forks source link

Add more Geography subclasses #26

Open benbovy opened 1 year ago

benbovy commented 1 year ago

Also:

Let's save the following for later:

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 40.24% and project coverage change: -6.20 :warning:

Comparison is base (e874e60) 56.07% compared to head (c516f38) 49.88%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #26 +/- ## ========================================== - Coverage 56.07% 49.88% -6.20% ========================================== Files 5 7 +2 Lines 214 425 +211 Branches 95 189 +94 ========================================== + Hits 120 212 +92 - Misses 7 61 +54 - Partials 87 152 +65 ``` | [Impacted Files](https://codecov.io/gh/benbovy/spherely/pull/26?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Benoit+Bovy) | Coverage Δ | | |---|---|---| | [src/s2geography\_addons.cpp](https://codecov.io/gh/benbovy/spherely/pull/26?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Benoit+Bovy#diff-c3JjL3MyZ2VvZ3JhcGh5X2FkZG9ucy5jcHA=) | `20.58% <20.58%> (ø)` | | | [src/s2geography\_addons.hpp](https://codecov.io/gh/benbovy/spherely/pull/26?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Benoit+Bovy#diff-c3JjL3MyZ2VvZ3JhcGh5X2FkZG9ucy5ocHA=) | `33.33% <33.33%> (ø)` | | | [src/geography.cpp](https://codecov.io/gh/benbovy/spherely/pull/26?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Benoit+Bovy#diff-c3JjL2dlb2dyYXBoeS5jcHA=) | `45.79% <43.53%> (+3.86%)` | :arrow_up: | | [src/geography.hpp](https://codecov.io/gh/benbovy/spherely/pull/26?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Benoit+Bovy#diff-c3JjL2dlb2dyYXBoeS5ocHA=) | `95.65% <100.00%> (+8.15%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Benoit+Bovy). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Benoit+Bovy)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jorisvandenbossche commented 1 year ago

Maybe something to also discuss more generally: do we want / need all those subclasses?

benbovy commented 1 year ago

Maybe something to also discuss more generally: do we want / need all those subclasses?

Do you mean that alternatively we could expose only one Geography class (with a geometry type property) along with top-level creation functions for each kind of geometry?

I see two advantages of having subclasses:

One argument against subclasses is that it is a bit more code to maintain (maybe some duplicate logic here and there), but that's not much in my opinion.

benbovy commented 1 year ago

Another alternative would be to expose a unique class for Point vs. MultiPoint, LineString vs. MultiLineString, etc. like in s2geography.

benbovy commented 1 year ago

@jorisvandenbossche I'm moving forward with the implementation of the Geography subclasses here but this shouldn't prevent us from discussing how best to expose these (maybe in a separate issue?). I wouldn't mind refactoring things if needed.

benbovy commented 1 year ago

I haven't checked if shapely copies the input geometries when creating a collection... Here that's a requirement if we want to reuse s2geography::GeographyCollection (which owns its features).

I added a clone method to the spherely wrapper classes which makes it easier. I think it would make things even easier if this method was available in the s2geography classes as well.

There are a few other things that we would need to port into the s2geography classes (also address #27) before we can get rid of spherely's wrapper classes (which I'd be very happy to see!). I'll wait a bit more before opening an issue in the s2geography repository with a clear and detailed proposal.