Fix hillSlope1 at 1: not identifiable with covariate coefficients - #47
Merged
Merged
Conversation
estimateSpreadParams() (fireSense_SpreadFit.R:886-921 pre-fix) put hillSlope1, the spread link's slope, in the default DEoptim bounds with [0.2, 2]. With the link's linear predictor x = covariates %*% beta, hillSlope1 enters only as hillSlope1 * x, so scaling every covariate coefficient by k and dividing hillSlope1 by k leaves every prediction unchanged: it was never identifiable and let coefficients drift along that ridge. estimateSpreadParams() no longer emits hillSlope1. fireSenseUtils::.objfunSpreadFit() (>= 0.2.3.9047) reinserts hillSlope1 = 1 before evaluating the fit. The run event's ledger row gets it back too (new addHillSlope1ToLedger()), so an old ledger row keeps predicting with its own fitted hillSlope1 and a new one predicts with 1. A supplied upper/lower naming hillSlope1 is now a clear error, since DEoptim would silently fit and then ignore it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CwcjqqK59FmTJscyi7xUqv
fireSenseUtils' development moved to 0.2.3.9047 (unrelated PR) while PredictiveEcology/fireSenseUtils#93 was open, so its fix rebased onto 0.2.3.9048; match the floor here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CwcjqqK59FmTJscyi7xUqv
…ix/hillSlope1-fixed-at-1 # Conflicts: # NEWS.md
eliotmcintire
changed the base branch from
development
to
fix/youngage-exclusivity
September 28, 2026 17:49
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.
estimateSpreadParams() (fireSense_SpreadFit.R:886-921 before this change) put hillSlope1, the spread link's slope, in the default DEoptim bounds ([0.2, 2]). With the link's linear predictor x = covariates %*% beta, hillSlope1 enters only as hillSlope1 * x, so scaling every covariate coefficient by k and dividing hillSlope1 by k leaves every prediction unchanged: it was never identifiable, and letting DEoptim fit it let every coefficient drift along that ridge.
estimateSpreadParams() no longer emits hillSlope1; fireSenseUtils::.objfunSpreadFit() (>= 0.2.3.9049, PredictiveEcology/fireSenseUtils#93) reinserts hillSlope1 = 1 before evaluating the fit. The run event's ledger row gets it back too, via the new addHillSlope1ToLedger(), so an old ledger row keeps predicting with its own fitted hillSlope1 and a new one predicts with 1. A supplied upper/lower naming hillSlope1 is now a clear error rather than being silently fit and then discarded.
Verified with new/updated tests covering: the default bounds no longer contain hillSlope1, a supplied upper/lower naming hillSlope1 is refused, the run event's ledger row gets hillSlope1 = 1 right after maxAsymptote, and (test-ledgerPrediction.R) an old-style row keeps predicting with its own fitted value while a new row predicts with 1. Full module suite (SpaDES.core::convertToPackage() + testthat::test_local()), stacked on PR #46 (fix/youngage-exclusivity) with fireSenseUtils 0.2.3.9049 from the scratch lib: 153 test blocks / 515 passed, 0 failed.
Version 1.0.6.9020. This branch is stacked on #46 (fix/youngage-exclusivity), so this PR's base is that branch, not development; it will retarget automatically once #46 merges. Requires fireSenseUtils@development (>= 0.2.3.9049), from PredictiveEcology/fireSenseUtils#93, similarly stacked on fireSenseUtils#92 -- merge order: fireSenseUtils#92, fireSenseUtils#93, fireSense_SpreadFit#46, fireSense_SpreadFit#47.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CwcjqqK59FmTJscyi7xUqv