Add more Geography subclasses

[benbovy]

• LinearRing
  • There is no corresponding class in s2geography (yet) and no class in s2geometry that we can use directly, but it was relatively (hopefully) easy to expose such type based on S2LaxClosedPolylineShape (+ S2Region implementation copied from S2Polyline). I added here some code that we might consider moving to s2geography later if it's worth it.
  • The name "LinearRing" might be a bit misleading considering that the segments are geodesics... Should we keep this name for consistency with Shapely or choose another name?
• Polygon
  • add support for holes
  • allow duplicate start/end vertices
• MultiPoint
• MultiLineString
• MultiPolygon
• GeometryCollection

Also:

• Use various kinds of geometries elsewhere in the tests (predicates, etc.)

Let's save the following for later:

• support of empty - or full - geographies
• vectorized creation functions

[codecov]

Codecov Report

Patch coverage: 40.24% and project coverage change: -6.20 ⚠️

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

Additional details and impacted files

@@            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	Coverage Δ
src/s2geography_addons.cpp	20.58% <20.58%> (ø)
src/s2geography_addons.hpp	33.33% <33.33%> (ø)
src/geography.cpp	45.79% <43.53%> (+3.86%)	⬆️
src/geography.hpp	95.65% <100.00%> (+8.15%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[jorisvandenbossche]

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

[benbovy]

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:

• it is convenient for quick creation of a few simple features (perhaps more readable too)
• it is consistent with Shapely (is there any plan to eventually drop those subclasses in Shapely?)

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]

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

• this also departs from Shapely's API
• I'm wondering what is the actual cost in performance of reusing the same class for single features (i.e., the unique S2 object put into a 1-element std::vector), especially for trivial functions applied to large arrays of single features (a rather common use case?). I have no idea, though, maybe splitting the classes in s2geography would be premature optimization.

[benbovy]

@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]

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.

[benbovy]

Closing. Superseded by #51.