Skip to content

Extract simple features-like interface into a C++ only API#165

Merged
paleolimbot merged 88 commits into
mainfrom
c-api
May 3, 2022
Merged

Extract simple features-like interface into a C++ only API#165
paleolimbot merged 88 commits into
mainfrom
c-api

Conversation

@paleolimbot
Copy link
Copy Markdown
Collaborator

This is a bit ambitious, but was inspired by Python folks interested in having some of these operations accessible from Python (see geopandas/community#10). The approach here is to use C++ rather than define a C API, although a C API could probably be wrapped around it if C linking were ever needed.

In addition to potentially benefiting the Python community, I'm hoping that having a wider community interested in helping to maintain the C++ that interfaces with S2 since I am admittedly a self-taught plumber in the world of compiled code.

I think I found a good way to do this piecemeal while keeping the tests passing (define a NewGeography() method and use it on a growing number of functions). I'm pleasantly surprised at how well this went for the accessors, which are the hardest because they often require access to the underlying data (and the new API obscures the underlying data and relies mostly on the Shape and Region interfaces).

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 11, 2022

Codecov Report

❌ Patch coverage is 91.09875% with 128 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.93%. Comparing base (0c74988) to head (3d33c13).
⚠️ Report is 263 commits behind head on main.

Files with missing lines Patch % Lines
src/s2-geography/geography.cpp 58.33% 30 Missing ⚠️
src/s2-geography/accessors-geog.cpp 82.52% 18 Missing ⚠️
src/s2-geography/constructor.hpp 88.31% 18 Missing ⚠️
src/s2-geography/build.cpp 92.14% 15 Missing ⚠️
src/s2-constructors-formatters.cpp 94.83% 14 Missing ⚠️
src/s2-geography/accessors.cpp 91.24% 12 Missing ⚠️
src/s2-geography/coverings.cpp 77.77% 8 Missing ⚠️
src/s2-geography/linear-referencing.cpp 75.75% 8 Missing ⚠️
src/s2-transformers.cpp 95.71% 3 Missing ⚠️
src/s2-geography/distance.cpp 96.55% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
- Coverage   94.75%   93.93%   -0.82%     
==========================================
  Files          42       49       +7     
  Lines        3220     3463     +243     
==========================================
+ Hits         3051     3253     +202     
- Misses        169      210      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paleolimbot paleolimbot marked this pull request as ready for review April 22, 2022 18:21
@paleolimbot
Copy link
Copy Markdown
Collaborator Author

@edzer This is a pretty big change! The gist of it is that everything in the R package is now R-specific, and anything that didn't need R is now separated out (into something that will eventually live in its own repo). We had excellent test coverage before (and I added a bit of test coverage as I went and noticed that some cases weren't tested), but there's still a chance that some wires got crossed.

Would you prefer to check this before I merge into main or should we do that before the next release? Another big ish change that we might want to do before the next release is to update the underlying S2 version (a new version was released about two weeks ago).

@paleolimbot paleolimbot merged commit c623cbc into main May 3, 2022
@paleolimbot paleolimbot deleted the c-api branch May 3, 2022 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants