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

ENH: add GEOSGeom_transformXY to C API #563

Merged
merged 5 commits into from
Feb 13, 2022

Conversation

brendan-ward
Copy link
Contributor

@brendan-ward brendan-ward commented Feb 8, 2022

Resolves #560

Allows caller to provide a callback function to transform x,y values of all cordinates in a geometry and return a new geometry; z values are unchanged.

Many thanks to @dbaston for providing >95% of the implementation in #560; I mostly tweaked a few things and added tests / benchmarks.

I diverged a bit from the direction suggested in #560, specifically:

  • I retained the XY suffix on the function names (GEOSGeom_transformXY, GEOSTransformXYCallback); I think this provides a very clear signal to the user that these apply only to x,y coordinates and helps avoid future ambiguity if an XYZ form is ever added. This follows the precedent for some of the other XY coordinate sequence getters / setters. As a user of the library, the XY suffix has been helpful and appreciated.
  • I switched the order of inputs to be geometry, callback. This is consistent with GEOSSTRtree_query, etc
  • I made the GEOSTransformXYCallback return 1 on success, 0 on failure like several other functions in the C API. Notably, it diverges from GEOSQueryCallback which is void and GEOSDistanceCallback which is 0 on success and nonzero otherwise (which I found a bit confusing). We need a clear way to signal from this callback in case of error. If appropriate, we can make this match the precedent of GEOSDistanceCallback.

@brendan-ward
Copy link
Contributor Author

Benchmark against a more manual version of transforming coordinates (see benchmarks/capi/GEOSGeom_transformXYPerfTest.cpp):

Running bin/perf_capi_transformxy
Run on (8 X 2700 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 8192 KiB (x1)
Load Average: 2.43, 2.63, 2.62
---------------------------------------------------------------------------------
Benchmark                                       Time             CPU   Iterations
---------------------------------------------------------------------------------
BM_GEOSGeom_transformXY<10>                  1859 ns         1852 ns       357595
BM_Geom_from_transformed_coords<10>          2765 ns         2755 ns       218684
BM_GEOSGeom_transformXY<1000>               30624 ns        30465 ns        22822
BM_Geom_from_transformed_coords<1000>       78919 ns        78276 ns         8920
BM_GEOSGeom_transformXY<10000>             309715 ns       300664 ns         2396
BM_Geom_from_transformed_coords<10000>     726570 ns       721475 ns          945

I didn't put a lot of effort into optimizing the more manual approach (get ring, get sequence, update each x,y individually, build ring, build geometry), but it is pretty typical of how that might be approached using the C API.

@dbaston
Copy link
Member

dbaston commented Feb 8, 2022

So...easier and faster. Nice!

I have to hold the compiler's hand a bit in devirtualization to get numbers like yours (see dbaston@b5710e2). What compiler are you using?

2022-02-08T14:04:05-05:00
Running bin/perf_capi_transformxy
Run on (8 X 4023.38 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 0.63, 0.83, 1.42
---------------------------------------------------------------------------------
Benchmark                                       Time             CPU   Iterations
---------------------------------------------------------------------------------
BM_GEOSGeom_transformXY<10>                   118 ns          118 ns      5920940
BM_Geom_from_transformed_coords<10>           245 ns          245 ns      2880928
BM_GEOSGeom_transformXY<1000>                2716 ns         2715 ns       260802
BM_Geom_from_transformed_coords<1000>        7353 ns         7353 ns        93494
BM_GEOSGeom_transformXY<10000>              27651 ns        27648 ns        25659
BM_Geom_from_transformed_coords<10000>      70374 ns        70372 ns         9716

without above commit

2022-02-08T14:07:29-05:00
Running bin/perf_capi_transformxy
Run on (8 X 3823.2 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 0.68, 0.78, 1.28
---------------------------------------------------------------------------------
Benchmark                                       Time             CPU   Iterations
---------------------------------------------------------------------------------
BM_GEOSGeom_transformXY<10>                   137 ns          137 ns      5020423
BM_Geom_from_transformed_coords<10>           230 ns          230 ns      3045327
BM_GEOSGeom_transformXY<1000>                4565 ns         4565 ns       151309
BM_Geom_from_transformed_coords<1000>        7565 ns         7564 ns        90683
BM_GEOSGeom_transformXY<10000>              45313 ns        45311 ns        15303
BM_Geom_from_transformed_coords<10000>      75461 ns        75459 ns         9036

@brendan-ward
Copy link
Contributor Author

What compiler are you using?

I'm on MacOS 10.15; I believe I'm using clang

Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin19.6.0

@brendan-ward brendan-ward marked this pull request as ready for review February 8, 2022 19:16
@brendan-ward brendan-ward changed the title WIP: add GEOSGeom_transformXY to C API ENH: add GEOSGeom_transformXY to C API Feb 8, 2022
capi/geos_c.h.in Outdated Show resolved Hide resolved
capi/geos_c.h.in Outdated Show resolved Hide resolved
@dr-jts dr-jts added the Enhancement New feature or feature improvement. label Feb 9, 2022
@dbaston dbaston merged commit 0d9bdb0 into libgeos:main Feb 13, 2022
@brendan-ward brendan-ward deleted the add_coordtransform_capi branch February 15, 2022 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or feature improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Expose coordinate transformation in C API
3 participants