Fix climateYear shadowing, stale climate rasters, categorical resampling and rstLCC setup - #13
Merged
Merged
Conversation
No behaviour change: only commented-out code, an if (FALSE) block, comments, desc strings and Rmd text are touched. Several input descs had been passed positionally into sourceURL; they are now in desc. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds tests for `ageNonForest()`, `getCurrentClimate()`, `Init()`/`init`,
`.inputObjects()` and the two covariate-assembly events, on a 4x4 toy
landscape whose every covariate can be worked out by hand. The toy climate
encodes both the year and the cell in its values, so an assertion on a value
proves which layer of which variable was taken.
99 assertions, no skips. Verified to pass identically on `origin/development`
and on this branch, run the way CI runs them (convertToPackage + test_local).
Every substantive test was mutation-checked: 14 mutations of the module
source, 14 detected.
Five defects are recorded, none of them blessed as correct: each states the
INTENDED assertion wrapped in `expect_failure()` so that fixing the defect
trips the test, plus an assertion pinning the current state.
- `.inputObjects` leaves `rstLCC_RTM` NULL: the `suppliedElsewhere("rstLCCs")`
branch does `tail(sim$rstLCCs, 1)[[1]]`, but `rstLCCs` is not a declared
input and so is absent. `Init()` then errors on `.compareRas(rtm, NULL)`,
which makes the module unusable as shipped.
- `.inputObjects` cannot build `flammableRTM`: `defineFlammable(rstLCC, ...)`
uses a local from the other branch and a function the module does not import.
- The covariate tables do not advance with simulation time: `getCurrentClimate()`
only builds `currentClimateRasters` when it is NULL, so from year two on the
climate is frozen at `start(sim)` while the `year` column still tracks
`time(sim)` -- the table claims 2003 and carries 2001 climate.
- `climateYear` does not override `time(sim)`: the inner lapply's formal
`currentYear = time(sim)` shadows the outer `currentYear`.
- `lightningDays` is NaN in the ignition table.
- `Init()` realigns landcover with bilinear `postProcess()`, averaging
categorical class codes into non-classes and silently yielding an all-NA
landcoverDT.
They are pinned observations of fireSenseUtils internals, identical on origin/development and on this branch, not hand-derived truths.
…ing, rstLCC setup
A. getCurrentClimate(): the inner lapply FUN had a `currentYear = time(sim)`
formal that shadowed the outer binding, so sim$climateYear never had any
effect. Removed the formal so the outer value is used. `climateYear` is
compared numerically, so its declared metadata class is now "numeric".
B. getCurrentClimate(): the build was guarded by
`is.null(sim$currentClimateRasters)`, but currentClimateRasters is also a
declared OUTPUT, so it persisted and the block ran only once: every year
after the first silently used year-one climate. The guard is now keyed on
the year (mod$currentClimateYear), preserving the within-year caching.
C. Init(): postProcess(rstLCC_RTM, to = rasterToMatch) used the bilinear
default on a CATEGORICAL landcover raster, producing invented classes and
an all-NA landcoverDT. Now method = "near".
D. .inputObjects(): `rstLCCs` was branched on but not declared as an input, so
a user-supplied rstLCCs was NULL in the simList; it is now a declared input
and the branch is guarded by !suppliedElsewhere("rstLCC_RTM"). The
makeFireSenseLCC() call used studyArea=/rasterToMatch=, which the installed
signature rejects; now maskTo=/to=. defineFlammable() referenced a local
`rstLCC` bound only in the other branch; it now derives the raster from
sim$rstLCC_RTM the same way Init() does.
Regression tests for each: tests/testthat/test-getCurrentClimate.R,
test-realignCategorical.R, test-inputObjectsRstLCC.R, with a shared
helper-toyPredict.R of in-memory terra/data.table inputs (no network).
Collaborator
Author
|
On the open question about fireSense_dataPrepFit defines Indexing by |
…; add empty save event Removed, never read: parameters .plotInitialTime, .plotInterval, .saveInitialTime, .saveInterval; input fireSense_IgnitionFitted and its check in .inputObjects, which only ran for a Fit module name. whichModulesToPrepare now defaults to the three Predict modules that doEvent tests for. It named fireSense_EscapeFit, which nothing tests for. The save event emits a message and does nothing else. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013B4sgRg9EwyHaAQdDzUaQW
…Rasters alone Merge #12 first; this branch now contains it. Conflicts and how each was resolved: - inputs metadata: #12's text and removals, with #13's climateYear class ("numeric") and the new rstLCCs input. - getCurrentClimate() guard: #13's code. The two dead comments #12 removed stay removed. - getCurrentClimate() assignment: #13's mod$currentClimateYear line, without the dead trailing comment. - .inputObjects rstLCC block, both ends: #13's if (!suppliedElsewhere("rstLCC_RTM", sim)) wrapper. - tests/testthat/test-getCurrentClimate.R: every test from both sides. #12's climateYear expect_failure pin is replaced by #13's test of the fix. Correction to #13's stale-climate fix: the guard was keyed on the year alone, so it rebuilt currentClimateRasters from projectedClimateRasters even when another module had supplied it. In the project the climateYear module supplies it every year, preferring historical rasters. Now a supplied object is left alone, and only an object this module built is rebuilt when the year changes (mod$builtCurrentClimate). New test: supplied every year, left untouched, and the covariate tables carry the supplied values. It fails on #13 as pushed. #12's expect_failure pins for the defects fixed here are now ordinary expectations: rstLCC_RTM from rstLCCs, flammableRTM in .inputObjects, nearest-neighbour landcover, covariates advancing each year. The toy helper's rstLCC_RTM workaround is gone. The lightningDays pin stays; it is not fixed. "the climate rasters are rebuilt when the year changes" now runs through spades(), because mod only persists there. Removed propFlammable (input and assignment): nothing reads it. Version 1.0.4.9000. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013B4sgRg9EwyHaAQdDzUaQW
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.
Merge #12 first; this branch already contains it.
Four confirmed behaviour defects, each fixed minimally and each with a regression test that fails on
developmentand passes here. No dead-code removal, no refactoring: this is deliberately separate from #12.A —
climateYearhad no effect (variable shadowing)getCurrentClimate()computedcurrentYearfromsim$climateYear(dev l.309-313), but the innerlapplyFUN (dev l.322) declared a formalcurrentYear = time(sim), which shadowed it. Observed:climateYear = 2003withstart = 2001returned the year2001 MDC layer (101, 102, 103...).Fix: dropped the formal so the outer
currentYearis used, and moved the computation above the guard.climateYearis compared numerically (currentYear > max(availableYears)), so its declared metadata class changed from"character"to"numeric"(it was never used as"year2009"-style text anywhere in the module), andtime(sim)'s"unit"attribute is stripped.B — covariate tables never advanced with simulation time (most serious)
The whole block was wrapped in
if (is.null(sim$currentClimateRasters))(dev l.296-334).currentClimateRastersis also a declared output (dev l.150), so it persisted across years and the block ran once. Observed: a 2001→2003 run produced an ignition table labelledyear = 2003carryingMDC = 103.5, 105.5, 111.5— aggregated year2001 values. Silent.Fix: a supplied
currentClimateRastersis left alone. In the project theclimateYearmodule supplies it every year. Only a raster this module built is refreshed, and only when the wanted year changes. The module records that it built the object, and for which year, inmod. A test supplies a different raster each year and checks that the covariate tables carry the supplied values. Another poisons the cached raster and re-runs the event in the same year, to check it is not rebuilt.C —
Init()destroyed categorical landcover on realignmentpostProcess(sim$rstLCC_RTM, to = sim$rasterToMatch)(dev l.241-245) with nomethod=defaults to bilinear, on a categorical raster. Triggers whenever.compareRas()is FALSE. Observed: a misaligned 4x4 LCC produced 210, 114.5, 113.75, 66, 17.75..., andmakeLandcoverDT()then returned an all-NAlandcoverDTof the correct shape and column names — a silent failure.Fix:
method = "near".D — both branches of the
rstLCC_RTMsetup were dead ends.inputObjects()(dev l.509) branched onsuppliedElsewhere("rstLCCs", sim), butrstLCCswas not a declared input:suppliedElsewherereturned TRUE whilesim$rstLCCswas NULL, sotail(NULL, 1)[[1]]leftrstLCC_RTMNULL andInit()died inLandR::.compareRas(). The other branch calledmakeFireSenseLCC(studyArea=, rasterToMatch=, ...), which the installedfireSenseUtils0.2.3.9024 signature rejects (unused arguments). And dev l.546defineFlammable(rstLCC, ...)referenced a local bound only inside theelsebranch.Fix, in three parts:
rstLCCsdeclared as an input, class"list", matchingfireSense_dataPrepFit(oneSpatRasterper data year; its l.499 doesdefineFlammable(sim$rstLCCs[[dy]], ...)).makeFireSenseLCC(maskTo = sim$studyArea, to = sim$rasterToMatch, ...), perargs(fireSenseUtils::makeFireSenseLCC)and the sibling module's call.!suppliedElsewhere("rstLCC_RTM", sim)(the commented-out intent at dev l.508/533), anddefineFlammable()derives its raster fromsim$rstLCC_RTMusing the samepostProcess(..., method = "near")constructionInit()uses, instead of the unbound local.Open question for the owner
The
rstLCCsinterface is nowtail(sim$rstLCCs, 1)[[1]]— the last element, as the existing code assumed.fireSense_dataPrepFitnames its elementsyear<yyyy>and indexes them by year. If predict should pick the element matchingP(sim)$dataYearrather than the last one, that is a behaviour decision I did not make; say so and I will change it.propFlammableis removed in the merge with #12, because nothing reads it.Tests
tests/testthat/helper-toyPredict.R(4x4 in-memoryterralandscape, MDC stack whoseyear<yyyy>layer is constant atyyyy - 1900, smalldata.tables, no network), plustest-getCurrentClimate.R,test-realignCategorical.R,test-inputObjectsRstLCC.R.test-metadata.Rupdated for the two metadata changes. All 20 assertions pass; each fails ondevelopment; each fix was individually reverted to confirm its test goes red.