Bump min s2geography version to 0.2.0 (#81)

* bump s2geography min version to 0.2.0 * s2geography_INCLUDE_DIRS provided by s2geography * wheels: try remove s2geography patches * update fix-openssl header s2geography patch * clean-up * remove s2geography <0.2.0 backward compatibility * require pybind >= 2.11.0 The geoarrow module uses the `py::capsule` constructor added in 2.11.0. https://pybind11.readthedocs.io/en/stable/changelog.html#version-2-11-0-july-14-2023 * remove s2geography <0.2.0 backward compat (tests) * clone and extract properties: use GeographyKind
benbovy · Dec 5, 2024 · 3f58302 · 3f58302
1 parent d40a8d2
commit 3f58302
Show file tree

Hide file tree

Showing 16 changed files with 120 additions and 193 deletions.
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -41,7 +41,7 @@ jobs:
     env:
       ABSL_VERSION: "20240722.0"
       S2GEOMETRY_VERSION: "0.11.1"
-      S2GEOGRAPHY_VERSION: "0.1.2"
+      S2GEOGRAPHY_VERSION: "0.2.0"
       CXX_STANDARD: 17
     strategy:
       fail-fast: false

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -34,9 +34,8 @@ target_include_directories(s2::s2 INTERFACE ${OPENSSL_INCLUDE_DIR})
 
 find_package(s2geography CONFIG REQUIRED)
 if(${s2geography_FOUND})
-  # not yet provided by s2geography
-  # get_target_property(s2geography_INCLUDE_DIRS s2geography INTERFACE_INCLUDE_DIRECTORIES)
-  message(STATUS "Found s2geography v${s2geography_VERSION}")
+  get_target_property(s2geography_INCLUDE_DIRS s2geography INTERFACE_INCLUDE_DIRECTORIES)
+  message(STATUS "Found s2geography v${s2geography_VERSION}: ${s2geography_INCLUDE_DIRS}")
 else()
   message(FATAL_ERROR "Couldn't find s2geography")
 endif()

diff --git a/README.md b/README.md
@@ -19,8 +19,8 @@ This library is at an early stage of development.
 
 - Python
 - Numpy
-- [s2geography](https://github.com/paleolimbot/s2geography)
-- [s2geometry](https://github.com/google/s2geometry)
+- [s2geography](https://github.com/paleolimbot/s2geography) v0.2.0 or higher
+- [s2geometry](https://github.com/google/s2geometry) v0.11.1 or higher
 
 Additional requirements when building spherely from source:
 

diff --git a/ci/environment-dev.yml b/ci/environment-dev.yml
@@ -8,7 +8,7 @@ dependencies:
   - cmake
   - python
   - numpy
-  - pybind11
+  - pybind11>=2.11.0
   - scikit-build-core
   - ninja
   - pytest

diff --git a/ci/environment.yml b/ci/environment.yml
@@ -4,12 +4,12 @@ channels:
 dependencies:
   - cxx-compiler
   - s2geometry>=0.11.1
-  - s2geography>=0.1.2
+  - s2geography>=0.2.0
   - libabseil
   - cmake
   - python
   - numpy
-  - pybind11
+  - pybind11>=2.11.0
   - scikit-build-core
   - ninja
   - pytest

diff --git a/ci/install_3rdparty.cmd b/ci/install_3rdparty.cmd
@@ -89,9 +89,9 @@ curl -o s2geography.tar.gz -L https://github.com/paleolimbot/s2geography/archive
 tar -xf s2geography.tar.gz -C %SRC_DIR%
 
 rem TODO: remove when fixed in s2geography
+rem (https://github.com/paleolimbot/s2geography/pull/53)
 cd %SRC_DIR%/s2geography-%S2GEOGRAPHY_VERSION%
 patch -i %PROJECT_DIR%\ci\s2geography-add-openssl-as-requirement.patch
-patch -i %PROJECT_DIR%\ci\s2geography-add-include-dir.patch
 
 cmake -GNinja ^
     -S %SRC_DIR%/s2geography-%S2GEOGRAPHY_VERSION% ^

diff --git a/ci/install_3rdparty.sh b/ci/install_3rdparty.sh
@@ -101,13 +101,12 @@ build_install_dependencies(){
     rm -f s2geography.tar.gz
 
     # TODO: remove when fixed in s2geography
+    # (https://github.com/paleolimbot/s2geography/pull/53)
     cd $SRC_DIR/s2geography-$S2GEOGRAPHY_VERSION
     if [ "$(uname)" == "Darwin" ]; then
         patch -p1 < $PROJECT_DIR/ci/s2geography-add-openssl-as-requirement.patch
-        patch -p1 < $PROJECT_DIR/ci/s2geography-add-include-dir.patch
     else
         patch -p1 < /project/ci/s2geography-add-openssl-as-requirement.patch
-        patch -p1 < /project/ci/s2geography-add-include-dir.patch
     fi
 
     cmake -S $SRC_DIR/s2geography-$S2GEOGRAPHY_VERSION -B $S2GEOGRAPHY_BUILD_DIR \

diff --git a/ci/s2geography-add-openssl-as-requirement.patch b/ci/s2geography-add-openssl-as-requirement.patch
@@ -1,51 +1,27 @@
-From 82176ad1b811e92cc8ae8d8f7fc1d0a8b2e6325a Mon Sep 17 00:00:00 2001
+From ccac9b1ea8ef85678473f6f6825e482ca4c19ebc Mon Sep 17 00:00:00 2001
 From: Benoit Bovy <[email protected]>
-Date: Tue, 20 Aug 2024 14:56:33 +0200
-Subject: [PATCH] add openssl as requirement
+Date: Mon, 2 Dec 2024 15:52:58 +0100
+Subject: [PATCH] fix openssl header not found in specific cases
 
-Not only for special cases `BUNDLED` and `BREW` for `S2_SOURCE`. Using
-`S2_SOURCE=SYSTEM`, there may be cases where openssl and s2geometry were
-installed in different prefix paths (e.g., system-installed openssl and
-custom s2geometry installation). The change here should address those
-cases too.
+E.g., on MacOS with s2geometry built from source and installed in a
+custom directory and linked against openssl installed in an other,
+standard directory.
 ---
- CMakeLists.txt | 13 +++++--------
- 1 file changed, 5 insertions(+), 8 deletions(-)
+ CMakeLists.txt | 5 +++++
+ 1 file changed, 5 insertions(+)
 
 diff --git a/CMakeLists.txt b/CMakeLists.txt
-index f71ff49..518d124 100644
+index 5fb3e93..6d040ad 100644
 --- a/CMakeLists.txt
 +++ b/CMakeLists.txt
-@@ -114,11 +114,6 @@ macro(build_s2)
-
-   set_property(TARGET s2 PROPERTY CXX_STANDARD ${CMAKE_CXX_STANDARD})
-
--  # this might be needed since s2geometry includes it in general
--  # but not for any target explicilty?
--  find_package(OpenSSL)
--  target_include_directories(s2 INTERFACE ${OPENSSL_INCLUDE_DIR})
--
-   get_target_property(S2_VERSION_STRING s2 VERSION)
-   extract_s2_version(${S2_VERSION_STRING})
-   add_library(s2::s2 ALIAS s2)
-@@ -128,9 +123,6 @@ if(${S2GEOGRAPHY_S2_SOURCE} STREQUAL "CONDA")
-   set(S2_ROOT_DIR "$ENV{CONDA_PREFIX}")
-   set(S2_SOURCE "SYSTEM")
- elseif(${S2GEOGRAPHY_S2_SOURCE} STREQUAL "BREW")
--  # required for Homebrew installed s2geometry headers to find OpenSSL headers
--  find_package(OpenSSL)
--  include_directories(${OPENSSL_INCLUDE_DIR})
-   set(S2_SOURCE "SYSTEM")
- else()
-   set(S2_SOURCE ${S2GEOGRAPHY_S2_SOURCE})
-@@ -159,4 +151,9 @@ if (MSVC AND NOT ${S2_SOURCE} STREQUAL "BUNDLED")
-   target_compile_options(s2::s2 INTERFACE /J)
+@@ -173,6 +173,11 @@ elseif(${S2_SOURCE} STREQUAL "SYSTEM")
+   endif()
  endif()
 
 +# this might be needed since s2geometry includes it in general
 +# but not for any target explicilty?
 +find_package(OpenSSL REQUIRED)
-+target_include_directories(s2::s2 INTERFACE ${OPENSSL_INCLUDE_DIR})
++target_include_directories(${s2_NOALIAS_TARGET} INTERFACE ${OPENSSL_INCLUDE_DIR})
 +
  # --- Abseil (bundled build not supported)
 

diff --git a/docs/environment.yml b/docs/environment.yml
@@ -9,7 +9,7 @@ dependencies:
   - cmake
   - make
   - s2geometry>=0.11.1
-  - s2geography>=0.1.2
+  - s2geography>=0.2.0
   - libabseil
   - sphinx
   - pydata-sphinx-theme>=0.8.1

diff --git a/pyproject.toml b/pyproject.toml
@@ -1,7 +1,7 @@
 [build-system]
 requires = [
     "scikit_build_core[rich]",
-    "pybind11",
+    "pybind11>=2.11",
 ]
 build-backend = "scikit_build_core.build"
 

diff --git a/src/geography.cpp b/src/geography.cpp
@@ -35,61 +35,52 @@ py::detail::type_info *PyObjectGeography::geography_tinfo = nullptr;
 */
 
 // TODO: May be worth moving this upstream as a `s2geog::Geography::clone()` virtual function
-std::unique_ptr<s2geog::Geography> clone_s2geography(const s2geog::Geography &geog);
-
-std::unique_ptr<s2geog::Geography> clone_s2geography(const s2geog::Geography &geog,
-                                                     GeographyType geog_type) {
+std::unique_ptr<s2geog::Geography> clone_s2geography(const s2geog::Geography &geog) {
     std::unique_ptr<s2geog::Geography> new_geog_ptr;
 
-    if (geog_type == GeographyType::Point || geog_type == GeographyType::MultiPoint) {
-        const auto &points = reinterpret_cast<const s2geog::PointGeography &>(geog).Points();
-        new_geog_ptr = std::make_unique<s2geog::PointGeography>(points);
+    switch (geog.kind()) {
+        case s2geog::GeographyKind::CELL_CENTER:
+        case s2geog::GeographyKind::POINT: {
+            const auto &points = reinterpret_cast<const s2geog::PointGeography &>(geog).Points();
+            return std::make_unique<s2geog::PointGeography>(points);
+        }
 
-    } else if (geog_type == GeographyType::LineString ||
-               geog_type == GeographyType::MultiLineString) {
-        const auto &polylines =
-            reinterpret_cast<const s2geog::PolylineGeography &>(geog).Polylines();
-        std::vector<std::unique_ptr<S2Polyline>> polylines_copy(polylines.size());
+        case s2geog::GeographyKind::POLYLINE: {
+            const auto &polylines =
+                reinterpret_cast<const s2geog::PolylineGeography &>(geog).Polylines();
+            std::vector<std::unique_ptr<S2Polyline>> polylines_copy(polylines.size());
 
-        auto copy_polyline = [](const std::unique_ptr<S2Polyline> &polyline) {
-            return std::unique_ptr<S2Polyline>(polyline->Clone());
-        };
+            auto copy_polyline = [](const std::unique_ptr<S2Polyline> &polyline) {
+                return std::unique_ptr<S2Polyline>(polyline->Clone());
+            };
 
-        std::transform(polylines.begin(), polylines.end(), polylines_copy.begin(), copy_polyline);
+            std::transform(
+                polylines.begin(), polylines.end(), polylines_copy.begin(), copy_polyline);
 
-        new_geog_ptr = std::make_unique<s2geog::PolylineGeography>(std::move(polylines_copy));
+            return std::make_unique<s2geog::PolylineGeography>(std::move(polylines_copy));
+        }
 
-    } else if (geog_type == GeographyType::Polygon || geog_type == GeographyType::MultiPolygon) {
-        const auto &poly = reinterpret_cast<const s2geog::PolygonGeography &>(geog).Polygon();
-        std::unique_ptr<S2Polygon> poly_ptr(poly->Clone());
-        new_geog_ptr = std::make_unique<s2geog::PolygonGeography>(std::move(poly_ptr));
+        case s2geog::GeographyKind::POLYGON: {
+            const auto &poly = reinterpret_cast<const s2geog::PolygonGeography &>(geog).Polygon();
+            std::unique_ptr<S2Polygon> poly_ptr(poly->Clone());
+            return std::make_unique<s2geog::PolygonGeography>(std::move(poly_ptr));
+        }
 
-    } else if (geog_type == GeographyType::GeometryCollection) {
-        const auto &features =
-            reinterpret_cast<const s2geog::GeographyCollection &>(geog).Features();
-        std::vector<std::unique_ptr<s2geog::Geography>> features_copy;
-        features_copy.reserve(features.size());
+        case s2geog::GeographyKind::GEOGRAPHY_COLLECTION: {
+            const auto &features =
+                reinterpret_cast<const s2geog::GeographyCollection &>(geog).Features();
+            std::vector<std::unique_ptr<s2geog::Geography>> features_copy;
+            features_copy.reserve(features.size());
 
-        for (const auto &feature_ptr : features) {
-            features_copy.push_back(clone_s2geography(*feature_ptr));
+            for (const auto &feature_ptr : features) {
+                features_copy.push_back(clone_s2geography(*feature_ptr));
+            }
+            return std::make_unique<s2geog::GeographyCollection>(std::move(features_copy));
         }
-        new_geog_ptr = std::make_unique<s2geog::GeographyCollection>(std::move(features_copy));
-    }
 
-    return new_geog_ptr;
-}
-
-std::unique_ptr<s2geog::Geography> clone_s2geography(const s2geog::Geography &geog) {
-    if (const auto *ptr = dynamic_cast<const s2geog::PointGeography *>(&geog); ptr) {
-        return clone_s2geography(geog, GeographyType::Point);
-    } else if (const auto *ptr = dynamic_cast<const s2geog::PolylineGeography *>(&geog); ptr) {
-        return clone_s2geography(geog, GeographyType::LineString);
-    } else if (const auto *ptr = dynamic_cast<const s2geog::PolygonGeography *>(&geog); ptr) {
-        return clone_s2geography(geog, GeographyType::Polygon);
-    } else if (const auto *ptr = dynamic_cast<const s2geog::GeographyCollection *>(&geog); ptr) {
-        return clone_s2geography(geog, GeographyType::GeometryCollection);
-    } else {
-        throw py::type_error("unknown geography type");
+        default: {
+            throw py::type_error("clone: s2geography kind not implemented");
+        }
     }
 }
 
@@ -98,11 +89,11 @@ std::unique_ptr<s2geog::Geography> clone_s2geography(const s2geog::Geography &ge
 */
 
 std::unique_ptr<s2geog::Geography> Geography::clone_geog() const {
-    return clone_s2geography(*m_s2geog_ptr, m_geog_type);
+    return clone_s2geography(geog());
 }
 
 Geography Geography::clone() const {
-    auto new_geog_ptr = clone_s2geography(*m_s2geog_ptr, m_geog_type);
+    auto new_geog_ptr = clone_s2geography(geog());
 
     // skip extract properties
     auto new_object = Geography();
@@ -115,50 +106,68 @@ Geography Geography::clone() const {
 }
 
 void Geography::extract_geog_properties() {
-    if (const auto *ptr = downcast_geog<s2geog::PointGeography>(); ptr) {
-        if (ptr->Points().empty()) {
-            m_is_empty = true;
-        }
-        if (ptr->Points().size() <= 1) {
-            m_geog_type = GeographyType::Point;
-        } else {
-            m_geog_type = GeographyType::MultiPoint;
-        }
-    } else if (const auto *ptr = downcast_geog<s2geog::PolylineGeography>(); ptr) {
-        if (ptr->Polylines().empty()) {
-            m_is_empty = true;
-        }
-        if (ptr->Polylines().size() <= 1) {
-            m_geog_type = GeographyType::LineString;
-        } else {
-            m_geog_type = GeographyType::MultiLineString;
-        }
-    } else if (const auto *ptr = downcast_geog<s2geog::PolygonGeography>(); ptr) {
-        const auto &s2poly_ptr = ptr->Polygon();
-        // count the outer shells (loop depth = 0, 2, 4, etc.)
-        int n_outer_shell_loops = 0;
-
-        for (int i = 0; i < s2poly_ptr->num_loops(); i++) {
-            if ((s2poly_ptr->loop(i)->depth() % 2) == 0) {
-                n_outer_shell_loops++;
+    switch (geog().kind()) {
+        case s2geog::GeographyKind::CELL_CENTER:
+        case s2geog::GeographyKind::POINT: {
+            auto ptr = cast_geog<s2geog::PointGeography>();
+            if (ptr->Points().empty()) {
+                m_is_empty = true;
+            }
+            if (ptr->Points().size() <= 1) {
+                m_geog_type = GeographyType::Point;
+            } else {
+                m_geog_type = GeographyType::MultiPoint;
             }
+            break;
         }
 
-        if (n_outer_shell_loops == 0) {
-            m_is_empty = true;
+        case s2geog::GeographyKind::POLYLINE: {
+            auto ptr = cast_geog<s2geog::PolylineGeography>();
+            if (ptr->Polylines().empty()) {
+                m_is_empty = true;
+            }
+            if (ptr->Polylines().size() <= 1) {
+                m_geog_type = GeographyType::LineString;
+            } else {
+                m_geog_type = GeographyType::MultiLineString;
+            }
+            break;
         }
-        if (n_outer_shell_loops <= 1) {
-            m_geog_type = GeographyType::Polygon;
-        } else {
-            m_geog_type = GeographyType::MultiPolygon;
+
+        case s2geog::GeographyKind::POLYGON: {
+            auto ptr = cast_geog<s2geog::PolygonGeography>();
+            const auto &s2poly_ptr = ptr->Polygon();
+            // count the outer shells (loop depth = 0, 2, 4, etc.)
+            int n_outer_shell_loops = 0;
+
+            for (int i = 0; i < s2poly_ptr->num_loops(); i++) {
+                if ((s2poly_ptr->loop(i)->depth() % 2) == 0) {
+                    n_outer_shell_loops++;
+                }
+            }
+
+            if (n_outer_shell_loops == 0) {
+                m_is_empty = true;
+            }
+            if (n_outer_shell_loops <= 1) {
+                m_geog_type = GeographyType::Polygon;
+            } else {
+                m_geog_type = GeographyType::MultiPolygon;
+            }
+            break;
         }
-    } else if (const auto *ptr = downcast_geog<s2geog::GeographyCollection>(); ptr) {
-        if (ptr->Features().empty()) {
-            m_is_empty = true;
+
+        case s2geog::GeographyKind::GEOGRAPHY_COLLECTION: {
+            auto ptr = cast_geog<s2geog::GeographyCollection>();
+            if (ptr->Features().empty()) {
+                m_is_empty = true;
+            }
+            m_geog_type = GeographyType::GeometryCollection;
+            break;
         }
-        m_geog_type = GeographyType::GeometryCollection;
-    } else {
-        m_geog_type = GeographyType::None;
+
+        default:
+            m_geog_type = GeographyType::None;
     }
 }
 

diff --git a/src/geography.hpp b/src/geography.hpp
@@ -75,9 +75,9 @@ class Geography {
         return *m_s2geog_ptr;
     }
 
-    template <class T, std::enable_if_t<std::is_base_of_v<s2geog::Geography, T>, bool> = true>
-    inline const T* downcast_geog() const {
-        return dynamic_cast<const T*>(&geog());
+    template <class T>
+    inline const T* cast_geog() const noexcept {
+        return reinterpret_cast<const T*>(&geog());
     }
 
     inline const s2geog::ShapeIndexGeography& geog_index() {
-Original file line number
+Diff line change
@@ Expand Up / @@ -8,7 +8,7 @@ dependencies: @@
       - cmake
       - python
       - numpy
-      - pybind11
+      - pybind11>=2.11.0
       - scikit-build-core
       - ninja
       - pytest
@@ Expand Down @@