Repository navigation
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 extends sea-level handling to support an additional reconstruction dataset (Clark 2025), and updates land-mask generation to optionally derive sea level from a named dataset rather than always requiring numeric inputs.
Changes:
- Update
make_land_mask()to accept a dataset identifier (defaulting to Spratt 2016) and compute sea level automatically. - Update internal
get_sea_level()to supportdataset = c("spratt2016", "clark2025")and use internal datasets. - Add a unit test for
make_land_mask()and update package docs/metadata (NEWS, Rd, WORDLIST, DESCRIPTION); add/organize data-raw scripts and sea-level source files.
Reviewed changes
Copilot reviewed 8 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
R/make_land_mask.R |
Changes API/defaults to allow sea level to be derived from a named dataset. |
R/get_sea_level.R |
Adds dataset selection and switches implementation to use internal datasets. |
tests/testthat/test_make_landmask.R |
Adds basic test coverage for land-mask creation. |
man/make_land_mask.Rd |
Updates rendered documentation for make_land_mask() usage/args. |
man/get_sea_level.Rd |
Updates rendered documentation for get_sea_level() signature/args. |
NEWS.md |
Notes the new Clark2025 sea-level dataset option. |
inst/WORDLIST |
Adjusts spelling whitelist entries. |
DESCRIPTION |
Bumps development version. |
data-raw/make_data/make_internal_datasets.R |
Consolidates internal dataset generation (including new sea-level datasets). |
data-raw/make_data/dataset_list_included.R |
Removes old dataset-list generation script (superseded by the consolidated script). |
data-raw/data_files/sea_level_spratt2016.txt |
Adds Spratt 2016 sea-level source data under data-raw. |
Files not reviewed (2)
- man/get_sea_level.Rd: Language not supported
- man/make_land_mask.Rd: Language not supported
Comments suppressed due to low confidence (4)
R/make_land_mask.R:39
- Changing the default from sea_level = NULL to a string means callers passing sea_level = NULL (previously meaning “auto-compute”) will now fail with "sea_level should be numeric". Consider keeping NULL as a backward-compatible alias for the default dataset, or explicitly supporting NULL in the new validation path.
make_land_mask <- function(relief_rast, time_bp, sea_level = "spratt2016") {
# if sea_level is a character, check that it is either spratt or clark
if (is.character(sea_level)) {
if (!sea_level %in% c("spratt2016", "clark2025")) {
stop("sea_level should be either 'spratt2016' or 'clark2025'")
}
sea_level <- get_sea_level(time_bp = time_bp, dataset = sea_level)
}
#now sea level should be numeric and the same length as time_bp
if (!is.numeric(sea_level)) {
stop("sea_level should be numeric")
}
R/get_sea_level.R:28
get_sea_level()now referencesspratt2016/clark2025objects, but there is no R-level definition for them in the package sources (only in data-raw). If these datasets are not included in the installed package’s internal data (e.g., R/sysdata.rda), this will error at runtime with “object not found”. Ensure these objects are shipped with the package (and updated in this PR), or fall back to loading from inst/extdata when missing.
get_sea_level <- function(time_bp, dataset = "spratt2016") {
dataset <- match.arg(dataset, c("spratt2016", "clark2025"))
if (dataset == "spratt2016") {
# check that time is not too old for the dataset
if (any(time_bp < -798000)) {
stop("spratt2016 only reached -798,000 years BP")
}
sea_level_info <- spratt2016
} else if (dataset == "clark2025") {
if (any(time_bp < -4882000)) {
stop("clark2025 only reached -4,882,000 years BP")
}
sea_level_info <- clark2025
}
R/get_sea_level.R:26
- The range checks only guard against times that are too old, but they don’t reject future values (time_bp > 0) and they don’t handle NA values (any(...) can yield NA and error in
if). Since the datasets are reconstructions for the past, add validation fortime_bp > 0and usena.rm = TRUE(or an explicit NA check) in theseany()calls.
if (dataset == "spratt2016") {
# check that time is not too old for the dataset
if (any(time_bp < -798000)) {
stop("spratt2016 only reached -798,000 years BP")
}
sea_level_info <- spratt2016
} else if (dataset == "clark2025") {
if (any(time_bp < -4882000)) {
stop("clark2025 only reached -4,882,000 years BP")
}
R/get_sea_level.R:40
- The rescaling assumes there is exactly one row with time_bp == 0. If there is no exact 0 (or multiple),
sea_level_info$sea_level[sea_level_info$time_bp == 0]can be length 0/ >1 and break recycling (potentially returning length-0 output). Use a safer baseline (e.g., interpolate at 0, or take the value at the closest time to 0) and assert a single scalar baseline.
sea_level <- stats::approx(
x = sea_level_info$time_bp,
y = sea_level_info$sea_level,
xout = time_bp
)$y
# rescale to have 0 for 0kBP
sea_level <- sea_level - sea_level_info$sea_level[sea_level_info$time_bp == 0]
return(sea_level)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+19
to
+21
| #' @param sea_level sea level at the time of interest. It can be set to | ||
| #' "Spratt2016" (the default) or "Clark2025" to automatically compute the | ||
| #' level from one of those two datasets. |
| \item{sea_level}{sea level at the time of interest (if left to NULL, this is | ||
| computed using Spratt 2016)} | ||
| \item{sea_level}{sea level at the time of interest. It can be set to | ||
| "Spratt2016" (the default) or "Clark2025" to automatically compute the |
Comment on lines
88
to
96
| cheatsheet | ||
| chelsa | ||
| ci | ||
| codecov | ||
| clark | ||
| com | ||
| config | ||
| coords | ||
| csv | ||
| cv | ||
| dir | ||
| discretisation |
| # test that we can create landmasks correctly | ||
| test_that("make_land_mask works", { | ||
| relief_rast <- terra::rast(matrix(c(0, 1, 2, 3, 4, 5), nrow = 2)) | ||
| time_bp <- c(1000, 2000) |
Comment on lines
4
to
+5
| and max temperature | ||
| * Add Clark2025 as a possible sea level dataset. |
Comment on lines
6
to
10
| #' year ago). | ||
| #' | ||
| #' @param time_bp the time of interest | ||
| #' @param dataset the dataset to use, either "spratt2016" or "clark2025" | ||
| #' @returns a vector of sea levels in meters from present level |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #84 +/- ##
==========================================
+ Coverage 87.47% 87.54% +0.07%
==========================================
Files 61 61
Lines 1876 1887 +11
==========================================
+ Hits 1641 1652 +11
Misses 235 235 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Comment on lines
+27
to
+39
| make_land_mask <- function(relief_rast, time_bp, sea_level = "Spratt2016") { | ||
| # if sea_level is a character, check that it is either spratt or clark | ||
| if (is.character(sea_level)) { | ||
| if (!sea_level %in% c("Spratt2016", "Clark2025")) { | ||
| stop("sea_level should be either 'Spratt2016' or 'Clark2025'") | ||
| } | ||
| sea_level <- get_sea_level(time_bp = time_bp, dataset = sea_level) | ||
| } | ||
|
|
||
| # now sea level should be numeric and the same length as time_bp | ||
| if (!is.numeric(sea_level)) { | ||
| stop("sea_level should be numeric") | ||
| } |
Comment on lines
+19
to
+21
| #' @param sea_level sea level at the time of interest. It can be set to | ||
| #' "Spratt2016" (the default) or "Clark2025" to automatically compute the | ||
| #' level from one of those two datasets. |
Comment on lines
+15
to
+27
| get_sea_level <- function(time_bp, dataset = "Spratt2016") { | ||
| dataset <- match.arg(dataset, c("Spratt2016", "Clark2025")) | ||
| if (dataset == "Spratt2016") { | ||
| # check that time is not too old for the dataset | ||
| if (any(time_bp < -798000)) { | ||
| stop("Spratt2016 only reached -798,000 years BP") | ||
| } | ||
| sea_level_info <- spratt2016 | ||
| } else if (dataset == "Clark2025") { | ||
| if (any(time_bp < -4882000)) { | ||
| stop("Clark2025 only reached -4,882,000 years BP") | ||
| } | ||
| sea_level_info <- clark2025 |
| )$y | ||
| # rescale to have 0 for 0kBP | ||
| sea_level <- sea_level - sea_level_info$SeaLev_longPC1[1] | ||
| sea_level <- sea_level - sea_level_info$sea_level[sea_level_info$time_bp == 0] |
* Initial plan * fix review thread feedback for sea level handling * refine review-fix tests * polish sea level tests * some comments --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Andrea Manica <am315@cam.ac.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.