New transformer: s2_convex_hull #151
Conversation
Codecov Report
@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 94.45% 94.50% +0.04%
==========================================
Files 40 40
Lines 2814 2838 +24
==========================================
+ Hits 2658 2682 +24
Misses 156 156
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is awesome...thank you! I'm going to request a few changes, mostly to make it more similar to the other functions in s2-transformers.cpp. Also a few suggestions that might help you simplify things!
I don't see any tests here...could you add one to test-s2-transformers.R?
R/s2-transformers.R
Outdated
| # @rdname s2_boundary | ||
| #' @export | ||
| s2_convex_hull <- function(x, options = s2_options() ) { | ||
| new_s2_xptr( cpp_s2_convex_hull(as_s2_geography(x), options), "s2_geography") |
There was a problem hiding this comment.
| new_s2_xptr( cpp_s2_convex_hull(as_s2_geography(x), options), "s2_geography") | |
| new_s2_xptr(cpp_s2_convex_hull(as_s2_geography(x), options), "s2_geography") |
src/s2-transformers.cpp
Outdated
| @@ -165,6 +166,7 @@ std::unique_ptr<Geography> rebuildGeography(S2ShapeIndex* index, | |||
| ); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Could you remove this newline (to minimize the diff)?
src/s2-transformers.cpp
Outdated
| GeographyOperationOptions options(s2options); | ||
| GeographyOperationOptions::LayerOptions layerOptions = options.layerOptions(); | ||
| S2Builder::Options buillderOptions = options.builderOptions(); | ||
|
|
||
| // create the builder | ||
| S2Builder builder(buillderOptions); | ||
|
|
||
| // create the data structures that will contain the output | ||
| std::vector<S2Point> points; | ||
| std::vector<std::unique_ptr<S2Polyline>> polylines; | ||
| std::unique_ptr<S2Polygon> polygon = absl::make_unique<S2Polygon>(); |
There was a problem hiding this comment.
See below - you won't need the Builder here so you can remove these lines.
src/s2-transformers.cpp
Outdated
| item = geog[i]; | ||
| if (item != R_NilValue) { | ||
| Rcpp::XPtr<Geography> feature(item); | ||
| feature->BuildShapeIndex(&index); |
There was a problem hiding this comment.
I don't think you need the index here so I think you can remove this line (and the one above where you declare it).
src/s2-transformers.cpp
Outdated
| if (item != R_NilValue) { | ||
| Rcpp::XPtr<Geography> feature(item); | ||
| feature->BuildShapeIndex(&index); | ||
| //std::cout << typeid(feature).name() << "\n"; |
There was a problem hiding this comment.
Could you remove this debug comment?
src/s2-transformers.cpp
Outdated
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of using the builder, I think you can just use return XPtr<Geography>(new PolygonGeography(std::move(convexHullQuery.GetConvexHull()))).
src/s2-transformers.cpp
Outdated
|
|
||
|
|
||
| // [[Rcpp::export]] | ||
| List cpp_s2_convex_hull(List geog, List s2options) { |
There was a problem hiding this comment.
The way you've implemented this is as an aggregate function...could you change the name to reflect this? (Maybe cpp_s2_convex_hull_agg()?)
src/s2-transformers.cpp
Outdated
| } | ||
| } else if (feature->GeographyType() == Geography::Type::GEOGRAPHY_COLLECTION) { | ||
| if (!feature->IsEmpty()) { | ||
| // TODO: recursion? |
There was a problem hiding this comment.
I haven't implemented a way to loop over the elements of a collection yet...Could you use Rcpp::stop() here instead to error when this happens?
…message thrown is case of geometry_collection; tests added
|
Hi @paleolimbot , |
|
This is really fantastic...I really appreciate the effort you put in to make the edits! Thank you again! I pushed a small fix to one of the tests to make it more likely to pass on more systems. |
|
Thanks to you! Your suggestions were really helpful. |
OK, I tried... It works (except on geometry collections - I gave up) but I'm afraid my coding may not be of the quality of other functions.
Please, dear maintainer, review carefully ;-)