Faster extract #80
Faster extract #80
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR modernizes the codebase with a branch migration from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces performance optimizations to the extract functionality by implementing a faster version of location_slice_from_region_series. The changes focus on improving efficiency when extracting climate data for multiple locations and time slices.
Key changes:
- Added support for Common Era (time_ce) time specification in
location_seriesfunction - Refactored
location_slice_from_region_seriesto extract values more efficiently using vectorized operations - Updated internal functions to use
region_seriesdirectly for better performance
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test_location_series.R | Added test coverage for time_ce parameter handling |
| R/location_slice_from_region_series_fast.R | Refactored to use vectorized terra::extract operations and improved neighbor interpolation logic |
| R/location_series.R | Modified to call location_slice_from_region_series internally and handle time_ce conversion |
| R/region_series.R | Reorganized code to retrieve time axis earlier in the process |
| data-raw/location_slice_from_region_series_fast.R | Development version with debugging code and incomplete implementations |
| man/*.Rd | Updated roxygen documentation references and maintainer information |
| R/download_etopo_subset.R, R/bathy_to_spatraster.R | Changed marmap function references from bracketed links to backtick format |
| DESCRIPTION | Updated RoxygenNote version to 7.3.3 |
Comments suppressed due to low confidence (3)
R/location_slice_from_region_series_fast.R:192
- Typo in comment: "vales" should be "values".
R/location_slice_from_region_series_fast.R:214 - Trailing whitespace on an otherwise empty line. This should be removed for consistency with coding style.
R/location_slice_from_region_series_fast.R:164 - Unused variable
n_neighbours. The variable is assigned the valuencol(neighbours_list)but is never used afterwards in the function.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # if we are not using a buffer, we try to get all the values from | ||
| # specific locations directly | ||
| if (!buffer) { | ||
| browser() |
There was a problem hiding this comment.
Debug statement browser() should be removed before merging to production. This will cause execution to halt during normal operation.
| browser() |
|
|
||
|
|
There was a problem hiding this comment.
Excess trailing whitespace on blank lines. These empty lines contain only whitespace which should be removed for consistency with coding style.
R/location_series.R
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Trailing whitespace on empty lines. These should be removed for consistency with coding style.
| # do the same for time-ce | ||
| locations_ts_ce <- location_series( | ||
| x = locations[, c("longitude", "latitude")], | ||
| time_ce = c(-20000, -10000, -5000)+1950, |
There was a problem hiding this comment.
Missing space around + operator. The expression should be c(-20000, -10000, -5000) + 1950 with spaces around the plus sign for better readability and consistency with R style guidelines.
| time_ce = c(-20000, -10000, -5000)+1950, | |
| time_ce = c(-20000, -10000, -5000) + 1950, |
| y = neighbours_coords, | ||
| layer = neighbours_list$layer | ||
| ) | ||
| browser() |
There was a problem hiding this comment.
Debug statement browser() should be removed before merging to production. This will cause execution to halt during normal operation.
| browser() |
R/region_series.R
Outdated
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Excessive trailing whitespace on empty lines. These should be removed for consistency with coding style.
| ) | ||
| # convert from matrix (one row per location) to data.frame where first | ||
| # column is focal location, second column is id of each neighbour | ||
| n_neighbours <- ncol(neighbours_list) |
There was a problem hiding this comment.
Unused variable n_neighbours. The variable is assigned the value ncol(neighbours_list) but is never used afterwards in the function.
| n_neighbours <- ncol(neighbours_list) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #80 +/- ##
==========================================
+ Coverage 87.33% 87.45% +0.11%
==========================================
Files 61 61
Lines 1856 1873 +17
==========================================
+ Hits 1621 1638 +17
Misses 235 235 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
R/location_slice_from_region_series_fast.R:191
- Typo in comment: "vales" should be "values".
R/location_slice_from_region_series_fast.R:134 - Extra space in comment: "value column" should be "value column" (double space should be single space).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
R/location_series.R
Outdated
| directions = directions | ||
| ) | ||
|
|
||
| # TODO if we had time_ce, we should convert back from time_bp |
There was a problem hiding this comment.
This TODO comment is obsolete. The code immediately following (lines 128-132) already implements the conversion from time_bp back to time_ce when time_ce is not null. The TODO should be removed.
| # TODO if we had time_ce, we should convert back from time_bp |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
R/location_slice_from_region_series_fast.R:216
- Typo in comment: "has no method" is correct English (the change from "has not" to "has no" is appropriate).
R/location_slice_from_region_series_fast.R:177 - The comment mentions a "BUG" in terra. While this may be accurate, the comment should clarify whether this is a known workaround or if there's a tracking issue. Consider adding a link to the terra issue tracker if available, or remove the "BUG" label if this is simply documenting the required approach.
R/location_slice_from_region_series_fast.R:131 - The refactored code at line 131 introduces a bug when
xis a numeric vector (cell numbers). However, there is no test coverage for the numeric input path inlocation_slice_from_region_series. Consider adding a test case that passes cell numbers instead of a data.frame to ensure this code path works correctly.
R/location_slice_from_region_series_fast.R:131 - The variable
coordsis undefined whenxis numeric (not a data.frame). This line should usecoords_dfinstead oflocations_data[coords]to correctly extract coordinates for both data.frame and numeric inputs. Thecoords_dfvariable is properly set at lines 114-118 to handle both cases.
R/location_slice_from_region_series_fast.R:58 - The word "readd" is not a standard word. It should be "re-add" or better yet, "return" since the code is determining whether to return time_ce format instead of time_bp format in the output.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.