Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 88 commits into from
May 3, 2022
Merged

Conversation

paleolimbot
Copy link
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

codecov-commenter commented Feb 11, 2022

Codecov Report

Merging #165 (b93cc48) into main (0c74988) will decrease coverage by 1.03%.
The diff coverage is 90.59%.

@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
- Coverage   94.75%   93.71%   -1.04%     
==========================================
  Files          42       50       +8     
  Lines        3220     3471     +251     
==========================================
+ Hits         3051     3253     +202     
- Misses        169      218      +49     
Impacted Files Coverage Δ
R/data.R 100.00% <ø> (ø)
src/s2-geography/capi/status.cpp 0.00% <0.00%> (ø)
src/s2-geography/capi/tessellator.cpp 100.00% <ø> (ø)
src/s2-lnglat.cpp 96.55% <ø> (ø)
src/wk-c-utils.c 98.81% <ø> (ø)
src/s2-geography/geography.cpp 58.33% <58.33%> (ø)
src/s2-geography/linear-referencing.cpp 75.75% <75.75%> (ø)
src/s2-geography/coverings.cpp 77.77% <77.77%> (ø)
src/s2-geography/accessors-geog.cpp 82.52% <82.52%> (ø)
src/s2-geography/constructor.hpp 88.31% <88.31%> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c74988...b93cc48. Read the comment docs.

@paleolimbot paleolimbot marked this pull request as ready for review April 22, 2022 18:21
@paleolimbot
Copy link
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