Add centroid, boundary and convex_hull function (returning new Geography) #20

jorisvandenbossche · 2023-03-08T21:17:54Z

No description provided.

jorisvandenbossche · 2023-03-09T06:33:51Z

src/accessors-geog.cpp

+PyObjectGeography convex_hull(PyObjectGeography a) {
+    const auto& a_ptr = a.as_geog_ptr()->geog();
+    auto s2_obj = s2geog::s2_convex_hull(a_ptr);
+    auto geog_ptr = std::make_unique<Geography>(std::move(s2_obj));


So currently, this function returns instances of the base class Geography. If I change this to std::make_unique<Polygon>, then it actually returns a Polygon python subclass.

For the convex_hull case, I think I can be sure that this will always return a Polygon. But in general, do we need a method on s2geography::Geography to know which subtype it actually is (a PolygonGeography) in this case), so we can assert / known to which subclass to downcast on our side?

I think the subtype could be inferred from both num_shapes() and dimension() methods, except maybe for empty geographies. That said, a specific method might be indeed convenient and slightly more efficient as dimension() iterates over each shape in "multi" subclasses.

I'm not sure if we really need it, though. Do you have some cases in mind where this would need to be inferred at runtime?

For the convex hull case, s2geog::s2_convex_hull returns a s2geog::Geography pointer but it should probably return a s2geog::PolygonGeography pointer since this is what S2ConvexHullAggregator::Finalize() returns anyway.

I don't know if this is the same for s2, but with GEOS there are certainly geometry-returning functions that can return varying geometry types depending on the input (eg an intersection can basically return any geometry type)

I don't know if this is the same for s2, but with GEOS there are certainly geometry-returning functions that can return varying geometry types depending on the input (eg an intersection can basically return any geometry type)

If you know the type of the input(s) can you always determine the type of the output(s), or is there some geometry-returning functions where the output geometry type depends on some internal processing logic?

In the 1st case, we could use spherely::Geography::geog_type() returned from the inputs. Although I agree this might be worth of having such method available in s2geography, which would help getting rid of spherely::Geography wrapper classes that makes things quite confused.

If you know the type of the input(s) can you always determine the type of the output(s), or is there some geometry-returning functions where the output geometry type depends on some internal processing logic?

Yes, AFAIK that is the case. For example consider intersection of two polygons: if they overlap, you can get a Polygon or MultiPolygon, if they touch along a segment you get a LineString (or MultiLineString), and if they touch you get a Point (or MultiPoint), and if they both overlap in some area and touch in another area, you get a GeometryCollection.

jorisvandenbossche · 2023-03-09T09:23:34Z

src/pybind11.hpp

@@ -90,6 +90,10 @@ class PyObjectGeography : public py::object {
        return py::cast(std::move(geog_ptr));
    }

+    // static PyObjectGeography as_py_object(std::unique_ptr<T> geog_ptr) {
+    //     return static_cast<PyObjectGeography>(py::cast(std::move(geog_ptr)));
+    // }


I need to clean this up, but I thought to have as_py_object actually return PyObjectGeography instead of py::object, since it's currently not yet used, and in practice we need PyObjectGeography in the vectorized functions. But didn't get this to work ..

Something like this should work:

template <class T, std::enable_if<...>> static PyObjectGeography as_py_object(std::unique_ptr<T> geog_ptr) { auto pyobj = py::cast(std::move(geog_ptr)); auto pyobj_geog = static_cast<PyObjectGeography&>(pyobj); return std::move(pyobj_geog); }

Thanks, will give that a try. Are you OK with changing the existing as_py_object to do that? (I am not sure if we have a use case of the current version that returns py::object)

Yes sure, it is also more consistent to have a static method returning an instance of the same type IMO. (from_geog could be a better name than as_py_object).

src/accessors-geog.cpp

Co-authored-by: Benoit Bovy <[email protected]>

codecov · 2023-03-18T19:41:45Z

Codecov Report

Patch coverage: 44.73% and project coverage change: -1.46 ⚠️

Comparison is base (e874e60) 56.07% compared to head (91ce98f) 54.61%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
- Coverage   56.07%   54.61%   -1.46%     
==========================================
  Files           5        6       +1     
  Lines         214      249      +35     
  Branches       95      114      +19     
==========================================
+ Hits          120      136      +16     
  Misses          7        7              
- Partials       87      106      +19

Impacted Files	Coverage Δ
src/geography.cpp	`41.66% <ø> (-0.27%)`	⬇️
src/accessors-geog.cpp	`37.93% <37.93%> (ø)`
src/geography.hpp	`83.33% <50.00%> (-4.17%)`	⬇️
src/pybind11.hpp	`82.14% <75.00%> (-0.47%)`	⬇️
src/spherely.cpp	`57.14% <100.00%> (+7.14%)`	⬆️

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 · 2023-03-18T20:26:44Z

tests/test_accessors.py

+    actual = spherely.centroid(geog)
+    assert isinstance(actual, spherely.Point)
+    # TODO add some way of testing almost equality
+    # assert spherely.equals(actual, expected)


Testing is not super easy at the moment (we will have to develop some utilities to help with this).

Currently this gives failures like

> assert spherely.equals(actual, expected) E assert False E + where False = <built-in method equals of PyCapsule object at 0x7fea1738e790>(POINT (4.999999999999999 0), POINT (5 0)) E + where <built-in method equals of PyCapsule object at 0x7fea1738e790> = spherely.equals

So either we need to have a version of equals that does some rounding/snapping of coordinates (I think S2 should provide some options for that, which we probably want to expose).
Or more generally test based on the WKT repr, but then we also need to have a way to specify / reduce the precision of the WKT repr (#8)

I agree we need better helpers or features for testing (getting object coordinates would be useful too). This is short-term priority I think.

jorisvandenbossche · 2023-03-18T20:29:23Z

src/accessors-geog.cpp

+    const auto& a_ptr = a.as_geog_ptr()->geog();
+    auto s2_obj = s2geog::s2_boundary(a_ptr);
+    // TODO return specific subclass
+    auto geog_ptr = std::make_unique<spherely::Geography>(std::move(s2_obj));


Boundary can return various geography types (empty collection for point, multipoint for linestring, linestring for polygon), so for now using the generic Geography, which will also give this parent class at the python level. We will have to add some utility to infer the type and cast to correct subclass.

Yes, this can be done after #26.

jorisvandenbossche · 2023-03-18T20:30:17Z

@benbovy this should be ready I think. There are certainly some things to improve (easier testing, correct return class), but those can be left for follow-ups I think.

benbovy

Nice, LGTM thanks @jorisvandenbossche!

Could you just add the new functions in the API reference, please? And then I think we can merge this right away.

benbovy · 2023-03-20T09:57:34Z

src/accessors-geog.cpp

+    const auto& a_ptr = a.as_geog_ptr()->geog();
+    auto s2_obj = s2geog::s2_boundary(a_ptr);
+    // TODO return specific subclass
+    auto geog_ptr = std::make_unique<spherely::Geography>(std::move(s2_obj));


Yes, this can be done after #26.

benbovy · 2023-03-20T10:00:19Z

tests/test_accessors.py

+    actual = spherely.centroid(geog)
+    assert isinstance(actual, spherely.Point)
+    # TODO add some way of testing almost equality
+    # assert spherely.equals(actual, expected)


I agree we need better helpers or features for testing (getting object coordinates would be useful too). This is short-term priority I think.

jorisvandenbossche added 4 commits March 8, 2023 21:00

basic support for convex_hull

3d7dd92

Merge remote-tracking branch 'upstream/main' into concave_hull

496d6aa

add tests

4f65fdf

cleanup

0f5fb03

jorisvandenbossche commented Mar 9, 2023

View reviewed changes

from_geog method + return Polygon

1cd9376

benbovy changed the title ~~Add concave_hull function (returning new Geography)~~ Add convex_hull function (returning new Geography) Mar 10, 2023

benbovy reviewed Mar 10, 2023

View reviewed changes

src/accessors-geog.cpp Outdated Show resolved Hide resolved

jorisvandenbossche and others added 4 commits March 18, 2023 20:26

Update src/accessors-geog.cpp

82a7e71

Co-authored-by: Benoit Bovy <[email protected]>

Merge remote-tracking branch 'upstream/main' into concave_hull

3331305

update tests

5a5d714

linting

8f5a025

jorisvandenbossche added 2 commits March 18, 2023 21:21

add centroid and boundary

11b0fbb

add type annotations

91ce98f

jorisvandenbossche changed the title ~~Add convex_hull function (returning new Geography)~~ Add centroid, boundary and convex_hull function (returning new Geography) Mar 18, 2023

jorisvandenbossche commented Mar 18, 2023

View reviewed changes

benbovy reviewed Mar 20, 2023

View reviewed changes

jorisvandenbossche added 3 commits August 19, 2024 10:12

Merge remote-tracking branch 'upstream/main' into concave_hull

b899e07

add to api docs

c368271

Merge remote-tracking branch 'upstream/main' into concave_hull

22e1439

jorisvandenbossche merged commit 38d3bf9 into benbovy:main Aug 19, 2024
15 checks passed

jorisvandenbossche deleted the concave_hull branch August 19, 2024 14:24

jorisvandenbossche mentioned this pull request Aug 19, 2024

Issue with exact equality and roundtrip of lat/lng coordinates #40

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add centroid, boundary and convex_hull function (returning new Geography) #20

Add centroid, boundary and convex_hull function (returning new Geography) #20

jorisvandenbossche commented Mar 8, 2023

jorisvandenbossche Mar 9, 2023

benbovy Mar 9, 2023

jorisvandenbossche Mar 9, 2023

benbovy Mar 9, 2023

jorisvandenbossche Mar 9, 2023

jorisvandenbossche Mar 9, 2023

benbovy Mar 9, 2023

jorisvandenbossche Mar 9, 2023

benbovy Mar 9, 2023

codecov bot commented Mar 18, 2023 •

edited

Loading

jorisvandenbossche Mar 18, 2023

benbovy Mar 20, 2023

jorisvandenbossche Mar 18, 2023

benbovy Mar 20, 2023

jorisvandenbossche commented Mar 18, 2023

benbovy left a comment

benbovy Mar 20, 2023

benbovy Mar 20, 2023

Add centroid, boundary and convex_hull function (returning new Geography) #20

Add centroid, boundary and convex_hull function (returning new Geography) #20

Conversation

jorisvandenbossche commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Mar 18, 2023

benbovy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2023 •

edited

Loading