Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1137 +/- ##
==========================================
- Coverage 87.52% 87.52% -0.01%
==========================================
Files 55 55
Lines 7626 7634 +8
Branches 7626 7634 +8
==========================================
+ Hits 6675 6682 +7
Misses 649 649
- Partials 302 303 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new remaining_demand_absolute_tolerance parameter to address issue #1115, where the investment appraisal would fail with NaN errors when service demand remained constant across consecutive milestone years. The tolerance allows the model to treat negligibly small remaining demand as zero, preventing spurious failures.
Changes:
- Added
remaining_demand_absolute_toleranceparameter with default value of 1e-12 - Modified
is_any_remaining_demandfunction to use the tolerance when checking for unmet demand - Added validation function and comprehensive unit tests for the new parameter
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/model/parameters.rs | Defines the new remaining_demand_absolute_tolerance parameter with default value, validation function, and comprehensive unit tests |
| src/simulation/investment.rs | Updates is_any_remaining_demand to use the new tolerance parameter when checking if demand is met |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Absolute tolerance when checking if remaining demand is close enough to zero | ||
| #[serde(default = "default_remaining_demand_absolute_tolerance")] | ||
| pub remaining_demand_absolute_tolerance: Dimensionless, |
There was a problem hiding this comment.
The schemas/input/model.yaml file needs to be updated to include the new remaining_demand_absolute_tolerance parameter. As noted in the comment at line 90-91 of this file, when adding or changing a field in ModelParameters, the schema must also be updated. This parameter should be documented similar to other parameters like price_tolerance in the schema file.
| fn is_any_remaining_demand(demand: &DemandMap, absolute_tolerance: f64) -> bool { | ||
| demand.values().any(|flow| *flow > Flow(absolute_tolerance)) | ||
| } |
There was a problem hiding this comment.
The modified is_any_remaining_demand function lacks test coverage. A test should be added to verify the behavior with different tolerance values, including edge cases like:
- Demand exactly equal to tolerance (should return false)
- Demand slightly above tolerance (should return true)
- Demand slightly below tolerance (should return false)
- Demand with zero tolerance
This is important because this function is central to fixing the bug described in issue Investment appraisal fails when demand is equal in 2 consecutive years in (a slight adaptation of) the simple model #1115.
| #[rstest] | ||
| #[case(0.0, true)] // Valid minimum value (exactly zero) | ||
| #[case(1e-10, true)] // Valid very small positive value | ||
| #[case(1e-6, true)] // Valid default value |
There was a problem hiding this comment.
The comment says "Valid default value" but uses 1e-6 instead of the actual default value 1e-12 defined at line 82. Update this test case to use the correct default value of 1e-12.
1 participant
Description
This adds a new configurable model parameter demand_tolerance so that model won't fail if the remaining demand is negligible
Fixes #1115
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks