Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a C++ backend for vector grid generation to achieve a 2-3x speed improvement. The implementation includes three new C++ functions for generating grid polygons, IDs, and parsing INSPIRE IDs, with a new vector_grid_backend parameter allowing users to choose between the optimized C++ backend (now the default) or the legacy R/sfheaders backend.
Changes:
- Added C++ implementation for grid geometry generation, ID creation, and ID parsing
- Introduced
vector_grid_backendparameter to all grid generation functions with "cpp" as the default - Replaced R-based ID generation and parsing with C++ equivalents for performance
- Added comprehensive test coverage comparing C++ and sfheaders backends
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/grid_worker.cpp | Core C++ implementations for grid generation, ID generation, and ID parsing |
| src/RcppExports.cpp | Auto-generated Rcpp exports for C++ functions |
| R/vector_backend_cpp.R | R wrapper for C++ grid generation backend |
| R/backend_sequential.R | Integration of C++ backend into sequential grid generation workflow |
| R/inspire_grid.R | Added vector_grid_backend parameter to all inspire_grid methods |
| R/inspire_id_to_coords.R | Replaced R parsing logic with C++ implementation |
| R/utils_as_grid.R | Performance optimizations for sfheaders backend |
| tests/testthat/test-backend_cpp.R | Comprehensive test comparing C++ and sfheaders backends |
| DESCRIPTION | Added Rcpp dependency |
| NAMESPACE | Exported new C++ functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Fallback or try to parse 'EPSG:3035' string? | ||
| # For now default to 3035 as per original logic if missing | ||
| epsg <- 3035 |
There was a problem hiding this comment.
The hardcoded fallback to EPSG:3035 should be consistent with the behavior in other parts of the codebase and properly documented. Consider extracting this to a constant or helper function to avoid magic numbers.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NAMESPACE
Outdated
| S3method(inspire_grid,sf) | ||
| S3method(inspire_grid,sfc) | ||
| export(generate_ids_rcpp) | ||
| export(grid_worker_rcpp) |
There was a problem hiding this comment.
The function convert_inspire_ids_rcpp is documented with @export in R/RcppExports.R (line 61) but is missing from the NAMESPACE file. This will prevent the function from being exported. Either add export(convert_inspire_ids_rcpp) to NAMESPACE or remove the @export tag from the documentation if the function should remain internal.
| export(grid_worker_rcpp) | |
| export(grid_worker_rcpp) | |
| export(convert_inspire_ids_rcpp) |
NAMESPACE
Outdated
| export(inspire_id_format) | ||
| export(inspire_id_from_coords) | ||
| export(inspire_id_to_coords) | ||
| export(parse_inspire_ids_rcpp) |
There was a problem hiding this comment.
The function convert_inspire_ids_rcpp is documented with @export in R/RcppExports.R (line 61) but is missing from the NAMESPACE file. This will prevent the function from being exported. Either add export(convert_inspire_ids_rcpp) to NAMESPACE or remove the @export tag from the documentation if the function should remain internal.
| export(parse_inspire_ids_rcpp) | |
| export(parse_inspire_ids_rcpp) | |
| export(convert_inspire_ids_rcpp) |
| nzeros <- .tz_count(cellsize_m) | ||
| div <- 10^nzeros | ||
| size_lbl <- if (cellsize_m >= 1000) paste0(cellsize_m / 1000, "km") else paste0(cellsize_m, "m") |
There was a problem hiding this comment.
This code block for calculating nzeros, div, and size_lbl is duplicated in at least three places in this file (lines 207-209, 323-325, and 388-390). Consider extracting this into a helper function to improve maintainability and reduce duplication.
| ) | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra blank line on line 39 should be removed for consistency.
|
|
||
|
|
There was a problem hiding this comment.
Multiple consecutive blank lines (63-64) should be reduced to a single blank line for consistency.
| }) | ||
|
|
||
|
|
||
| test_that("inspire_id_to_coords warns on multiple CRSs", { |
There was a problem hiding this comment.
Extra blank line on line 119 should be removed for consistency.
|
|
||
| sprintf("CRS%.0fRES%.0fmN%.0fE%.0f", crs, cellsize_m, y_long, x_long) | ||
| } | ||
| # 3. Perform conversion based on format |
There was a problem hiding this comment.
Duplicate comment on line 72. The comment '# 3. Perform conversion based on format' appears both before and after the match.arg call, creating redundancy.
| # 3. Perform conversion based on format |
R/inspire_grid_from_ids.R
Outdated
| dsn = dsn, | ||
| layer = layer, | ||
| quiet = quiet, | ||
| vector_grid_backend = vector_grid_backend |
There was a problem hiding this comment.
Inconsistent indentation: lines 72-74 use extra spaces compared to line 75 (vector_grid_backend = vector_grid_backend). All parameters should be aligned consistently.
| dsn = dsn, | |
| layer = layer, | |
| quiet = quiet, | |
| vector_grid_backend = vector_grid_backend | |
| dsn = dsn, | |
| layer = layer, | |
| quiet = quiet, | |
| vector_grid_backend = vector_grid_backend |
| y = strtod(y_str.c_str(), NULL); | ||
|
|
||
| // Parse X (after E) | ||
| x = strtod(e_pos + 1, &end); |
There was a problem hiding this comment.
In the long-to-short conversion, when parsing the X coordinate after 'E', the end pointer is updated but not validated. Consider adding a check for trailing characters after parsing X to ensure complete consumption of the input string, similar to the validation done for short-to-long conversion (line 476) and the parse function (lines 274, 316).
No description provided.