SyncroSim v3.1 updates#1
Open
sarah-chisholm wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a complete SyncroSim add-on (stsimNestweb): package manifest and metadata, a pinned Conda environment, R transformers ( ChangesstsimNestweb package + tooling
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modelHabitat.R (2)
116-116:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHardcoded absolute path will break portability.
Same issue as line 80—
"D:/nestweb/Model-Inputs/Spatial/mean-decay.tif"is machine-specific and will fail in remote execution or on other systems.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modelHabitat.R` at line 116, Replace the hardcoded absolute path in the meanDecay assignment (the rast(...) call that creates variable meanDecay) with a portable construction: read the base inputs directory from a configuration variable or environment variable (e.g., inputs_dir or Sys.getenv("MODEL_INPUTS_DIR")) or build a project-relative path using file.path() or here::here(), then pass that constructed path to rast(); update the meanDecay assignment to use that config/env or relative path so the code works on other machines and in remote execution.
137-137:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHardcoded absolute path will break portability.
Third instance of the hardcoded
"D:/nestweb/"path. All three occurrences (lines 80, 116, 137) should be refactored to use configurable or relative paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modelHabitat.R` at line 137, The code uses a hardcoded absolute path "D:/nestweb/" in read_csv when building rawNestwebData (and two other occurrences at the same file), which breaks portability; replace the literal with a single configurable variable or a relative path helper (e.g., define nestweb_dir at top of script or use here::here()/file.path(getwd(), ...) and then call read_csv(file.path(nestweb_dir, tabularDataDir, "Habitat selection - full dataset (30.03.2022).csv"), show_col_types = FALSE) %>%); update all three occurrences (around the rawNestwebData/read_csv uses) to reference that variable/helper so the path is centralized and portable.
🧹 Nitpick comments (1)
src/modelHabitat.R (1)
83-105: 💤 Low valueComplex lookup construction—consider adding clarifying comments.
The
invalidHabitatLookupconstruction involves multiple left joins with self-generated expansion tables and an anti-join pattern. While functionally correct for expanding wildcard rows to explicit combinations, the logic is dense.Consider adding a brief comment explaining the intent: expanding partial/wildcard
InvalidHabitatentries to explicit species–stratum–state class combinations for raster masking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modelHabitat.R` around lines 83 - 105, The invalidHabitatLookup pipeline is dense and should get a short clarifying comment: above the chain that starts with InvalidHabitat and builds invalidHabitatLookup, add a one- or two-sentence comment stating that the code expands wildcard/partial InvalidHabitat rows into explicit Species–StateClass–Stratum combinations (by joining replicated name vectors from Stratum, StateClass and names(SpeciesID), using anti_join against the full expand_grid of Species/StateClassId/StratumId, then mapping names to IDs via lookup and computing StateClassStratumId) so readers understand why left_join, expand_grid, anti_join, lookup and the final bind_rows/unique steps are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modelHabitat.R`:
- Line 80: The read_csv call that assigns speciesCodes uses a hardcoded absolute
path "D:/nestweb", which breaks portability; update the call that constructs the
filepath (the speciesCodes <- read_csv(...) invocation that references
tabularDataDir) to build the path from a portable source instead (e.g., the
scenario/data directory, an environment variable, or a relative path helper) so
the code works across machines and remote runs—replace the absolute "D:/nestweb"
with a reference to a configurable base path (env var or scenario data folder)
and ensure tabularDataDir is appended via file.path.
- Around line 47-48: The variable assignment uses inconsistent column casing: in
modelHabitat.R the pipeline pulls "SpeciesId" into SpeciesID while elsewhere
(summarizeHabitat.R) code expects "SpeciesID"; confirm the actual column name
returned by datasheet(myScenario, "stsimNestweb_Species", includeKey = TRUE) and
make both callers use the same exact column name, updating the pull(...) call in
the SpeciesID assignment and the corresponding pull in summarizeHabitat.R so
they reference the identical column string (e.g., "SpeciesID" or "SpeciesId")
and keep the exported variable name SpeciesID consistent.
---
Outside diff comments:
In `@src/modelHabitat.R`:
- Line 116: Replace the hardcoded absolute path in the meanDecay assignment (the
rast(...) call that creates variable meanDecay) with a portable construction:
read the base inputs directory from a configuration variable or environment
variable (e.g., inputs_dir or Sys.getenv("MODEL_INPUTS_DIR")) or build a
project-relative path using file.path() or here::here(), then pass that
constructed path to rast(); update the meanDecay assignment to use that
config/env or relative path so the code works on other machines and in remote
execution.
- Line 137: The code uses a hardcoded absolute path "D:/nestweb/" in read_csv
when building rawNestwebData (and two other occurrences at the same file), which
breaks portability; replace the literal with a single configurable variable or a
relative path helper (e.g., define nestweb_dir at top of script or use
here::here()/file.path(getwd(), ...) and then call
read_csv(file.path(nestweb_dir, tabularDataDir, "Habitat selection - full
dataset (30.03.2022).csv"), show_col_types = FALSE) %>%); update all three
occurrences (around the rawNestwebData/read_csv uses) to reference that
variable/helper so the path is centralized and portable.
---
Nitpick comments:
In `@src/modelHabitat.R`:
- Around line 83-105: The invalidHabitatLookup pipeline is dense and should get
a short clarifying comment: above the chain that starts with InvalidHabitat and
builds invalidHabitatLookup, add a one- or two-sentence comment stating that the
code expands wildcard/partial InvalidHabitat rows into explicit
Species–StateClass–Stratum combinations (by joining replicated name vectors from
Stratum, StateClass and names(SpeciesID), using anti_join against the full
expand_grid of Species/StateClassId/StratumId, then mapping names to IDs via
lookup and computing StateClassStratumId) so readers understand why left_join,
expand_grid, anti_join, lookup and the final bind_rows/unique steps are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed4f7f6a-4247-44f8-b85d-a58b5e653879
📒 Files selected for processing (8)
README.mdpackage.xmlsrc/.metadatasrc/modelHabitat.Rsrc/package.xmlsrc/stsimnestweb.ymlsrc/summarizeHabitat.Rstsimnestweb.yml
💤 Files with no reviewable changes (1)
- package.xml
Comment on lines
+47
to
+48
| SpeciesID <- datasheet(myScenario, "stsimNestweb_Species", includeKey = TRUE) %>% | ||
| pull(SpeciesId, name = Name) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare SpeciesId vs SpeciesID usage across both R scripts
echo "=== modelHabitat.R ==="
rg -n 'SpeciesI[Dd]' src/modelHabitat.R
echo ""
echo "=== summarizeHabitat.R ==="
rg -n 'SpeciesI[Dd]' src/summarizeHabitat.RRepository: ApexRMS/stsimNestweb
Length of output: 1917
🏁 Script executed:
#!/bin/bash
# Search for schema definitions or documentation files that might define the Species datasheet structure
echo "=== Looking for schema files ==="
fd -e "json" -e "csv" -e "yaml" -e "yml" | grep -i "species\|schema" | head -20
echo ""
echo "=== Looking for test files that might use this datasheet ==="
fd -e "R" -e "r" | xargs rg -l "stsimNestweb_Species" | head -10
echo ""
echo "=== Check if there are any example data or vignettes ==="
find . -type f \( -name "*species*" -o -name "*example*" -o -name "*vignette*" \) | head -20Repository: ApexRMS/stsimNestweb
Length of output: 259
🏁 Script executed:
#!/bin/bash
# Search for any references to the column name in comments or documentation
echo "=== Looking for column name documentation ==="
rg -A 2 -B 2 "SpeciesId|SpeciesID" . --type r | grep -i "column\|field\|datasheet" -A 2 -B 2 | head -30
echo ""
echo "=== Check the datasheet call in both files for any hints ==="
rg -A 5 "datasheet.*stsimNestweb_Species" src/Repository: ApexRMS/stsimNestweb
Length of output: 1889
Fix naming inconsistency: SpeciesId vs SpeciesID column name mismatch.
modelHabitat.R (line 48) pulls SpeciesId (lowercase 'd'), while summarizeHabitat.R (line 28) pulls SpeciesID (uppercase 'D'). Both call the same datasheet, so one of these will fail at runtime. Verify the actual column name returned by datasheet(..., "stsimNestweb_Species", includeKey = TRUE) and update both files to use the correct name consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modelHabitat.R` around lines 47 - 48, The variable assignment uses
inconsistent column casing: in modelHabitat.R the pipeline pulls "SpeciesId"
into SpeciesID while elsewhere (summarizeHabitat.R) code expects "SpeciesID";
confirm the actual column name returned by datasheet(myScenario,
"stsimNestweb_Species", includeKey = TRUE) and make both callers use the same
exact column name, updating the pull(...) call in the SpeciesID assignment and
the corresponding pull in summarizeHabitat.R so they reference the identical
column string (e.g., "SpeciesID" or "SpeciesId") and keep the exported variable
name SpeciesID consistent.
| @@ -80,29 +80,29 @@ species <- HabitatModel$Name | |||
| speciesCodes <- read_csv(file.path("D:/nestweb", tabularDataDir, "species-codes.csv"), show_col_types = FALSE) | |||
There was a problem hiding this comment.
Hardcoded absolute path will break portability.
"D:/nestweb" is a machine-specific absolute path. This will fail on any other system or when deployed to remote execution environments (the transformer is configured with runContext="AllowRemote").
Consider using relative paths from the scenario's data folder, environment variables, or scenario-level configuration to specify data locations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modelHabitat.R` at line 80, The read_csv call that assigns speciesCodes
uses a hardcoded absolute path "D:/nestweb", which breaks portability; update
the call that constructs the filepath (the speciesCodes <- read_csv(...)
invocation that references tabularDataDir) to build the path from a portable
source instead (e.g., the scenario/data directory, an environment variable, or a
relative path helper) so the code works across machines and remote runs—replace
the absolute "D:/nestweb" with a reference to a configurable base path (env var
or scenario data folder) and ensure tabularDataDir is appended via file.path.
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
1 participant
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.
The stsimNestweb package was updated from the SyncroSim v2 package format to v3, which required significant restructuring of the package.xml: wrapper elements were removed, datasheet definitions were moved to the top level, transformer I/O declarations were simplified, and layout name attributes were replaced with type. Additional v3.1-specific fields were added including minSyncroSimVersion, condaEnvVersion, isStochasticTime, and updateProvider. Column naming convention was also updated from ID to Id suffix throughout the XML and R scripts.
The conda environment was modernized by removing the deprecated r-rgdal package, updating Python from 3.6 to 3.11, pinning r-rsyncrosim>=2.1.9, and adding missing direct dependencies (r-terra, r-lme4, r-glmmtmb). In the R scripts, all calls to the deprecated datasheetSpatRaster() function were replaced with the v3-compatible datasheet() %>% filter() %>% pull() %>% rast() pattern.
Summary by CodeRabbit
Documentation
New Features
Refactor