add remaining_demand_absolute_tolerance param by Aurashk · Pull Request #1137 · EnergySystemsModellingLab/MUSE2

Aurashk · 2026-02-19T21:14:25Z

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

Bug fix (non-breaking change to fix an issue)
New feature (non-breaking change to add functionality)
Refactoring (non-breaking, non-functional change to improve maintainability)
Optimization (non-breaking change to speed up the code)
Breaking change (whatever its nature)
Documentation (improve or add documentation)

Key checklist

All tests pass: $ cargo test
The documentation builds and looks OK: $ cargo doc
Update release notes for the latest release if this PR adds a new feature or fixes a bug
present in the previous release

Further checks

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

codecov · 2026-02-19T21:16:32Z

Codecov Report

❌ Patch coverage is 71.87500% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.46%. Comparing base (3d4b209) to head (f4026b4).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/investment.rs	46.66%	8 Missing ⚠️
src/model/parameters.rs	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1137      +/-   ##
==========================================
- Coverage   87.52%   87.46%   -0.07%     
==========================================
  Files          55       55              
  Lines        7626     7652      +26     
  Branches     7626     7652      +26     
==========================================
+ Hits         6675     6693      +18     
- Misses        649      657       +8     
  Partials      302      302

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

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_tolerance parameter with default value of 1e-12
Modified is_any_remaining_demand function 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.

Copilot · 2026-02-19T21:18:13Z

src/model/parameters.rs

+    /// 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,


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.

Copilot · 2026-02-19T21:18:14Z

src/simulation/investment.rs

+fn is_any_remaining_demand(demand: &DemandMap, absolute_tolerance: f64) -> bool {
+    demand.values().any(|flow| *flow > Flow(absolute_tolerance))
 }


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.

Copilot · 2026-02-19T21:18:14Z

src/model/parameters.rs

+    #[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


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.

alexdewar

Thanks for this!

A couple of changes:

I'd make the parameter a Flow value, because that's what it actually represents
I'd gate this option behind the please_give_me_broken_results option, because it's potentially pretty dangerous. As it stands, the user could set it to 100 or something, which would be nonsensical. We could give a warning/error if the user sets it to something unreasonably high, but that does require making a judgment about what values are appropriate. So I'd just assume that this tolerance is good enough for the foreseeable future and if it turns out that it isn't for a (reasonable) model someone is using, then we can consider changing the default. This is basically an expert-only option, I think.

alexdewar · 2026-02-23T15:26:11Z

src/model/parameters.rs

 define_unit_param_default!(default_price_tolerance, Dimensionless, 1e-6);
+define_unit_param_default!(
+    default_remaining_demand_absolute_tolerance,
+    Dimensionless,


I think this should actually be Flow. (Yes, it is a bit weird that we are using one number for any commodity flow type, but that seems to be what's needed!)

alexdewar · 2026-02-23T15:26:31Z

src/simulation/investment.rs

 /// Check whether there is any remaining demand that is unmet in any time slice
-fn is_any_remaining_demand(demand: &DemandMap) -> bool {
-    demand.values().any(|flow| *flow > Flow(0.0))
+fn is_any_remaining_demand(demand: &DemandMap, absolute_tolerance: f64) -> bool {


As above, this should be of type Flow.

tsmbland

Agree with @alexdewar's comments, but otherwise looks good.

Surprised we got away without this for so long, to be honest.

FWIW we have a similar parameter in MUSE1 (tolerance_unmet_demand) which is also an absolute tolerance. This is freely accessible to users and set by default at a much higher value of 0.1. The difference is that in MUSE1 this tolerance check happens after investments are complete, whereas here we're doing checks while investments are ongoing. Therefore, a large tolerance is dangerous not just because supply might end up lower than demand (as is the case in MUSE1), but that it most likely will end up lower than demand because investments will terminate early. Because of this, I agree with @alexdewar that this is not something that users should customise and we should guard it behind please_give_me_broken_results.

…oken_options set units of remaining_demand_absolute_tolerance to Flow

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-24T13:40:26Z

schemas/input/model.yaml

+    description: |
+      Absolute tolerance when checking if remaining demand is close enough to zero in the
+      investment cycle.
+    default: 1e-12


The validation currently rejects remaining_demand_absolute_tolerance values above the default unless please_give_me_broken_results is enabled, but the schema text doesn't mention this constraint. Either document this requirement in the schema description/notes (and other docs) or relax the validation to allow safe increases without forcing the broken-results flag.

Suggested change

default: 1e-12

default: 1e-12

notes: |

This is a very strict tolerance; the default value is recommended for most cases.

The validation currently rejects values larger than the default unless

`please_give_me_broken_results` is set to true. Increasing this tolerance may

lead to incorrect or misleading investment results and should only be done with

a full understanding of the implications.

Copilot · 2026-02-24T13:40:27Z

src/model/parameters.rs

+        "remaining_demand_absolute_tolerance must be a finite number greater than or equal to zero"
+    );
+
+    // we already checked value is positive, but if they are


This comment says the value was checked to be "positive", but the validation allows Flow(0.0) (non-negative). Consider updating the wording to avoid implying that zero is invalid.

Suggested change

// we already checked value is positive, but if they are

// we already checked value is non-negative, but if they are

Copilot · 2026-02-24T13:40:27Z

src/model/parameters.rs

+            "Setting a remaining_demand_absolute_tolerance higher than the default value \
+             of 1e-12 is potentially dangerous, set \
+             please_give_me_broken_results to true if you want to allow this.",


The expected error fragment in this test is whitespace-sensitive due to \ line continuations and indentation. The actual ensure! message inserts indentation spaces after {:e} (before "is potentially"), while this fragment inserts indentation spaces before "of 1e-12"; as a result, error_message.contains(...) can fail even when the correct error is produced. Consider asserting on a shorter, whitespace-robust fragment (e.g., containing "higher than the default value" and "please_give_me_broken_results") or normalizing whitespace before comparison.

Suggested change

"Setting a remaining_demand_absolute_tolerance higher than the default value \

of 1e-12 is potentially dangerous, set \

please_give_me_broken_results to true if you want to allow this.",

"please_give_me_broken_results",

Copilot · 2026-02-24T13:40:27Z

src/model/parameters.rs

+    /// Absolute tolerance when checking if remaining demand is close enough to zero
+    #[serde(default = "default_remaining_demand_absolute_tolerance")]
+    pub remaining_demand_absolute_tolerance: Flow,


The PR description mentions adding a configurable parameter demand_tolerance, but the implemented/configured parameter name is remaining_demand_absolute_tolerance (Rust + schema). Please align the PR description (and any user-facing docs/changelog, if applicable) with the actual parameter name to avoid confusion for model authors.

tsmbland · 2026-02-24T13:55:19Z

src/model/parameters.rs

+    let default_value = default_remaining_demand_absolute_tolerance();
+    if !allow_broken_options {
+        ensure!(
+            value <= default_value,


I'd change this to a == because it's probably not advisable for users to decrease it below the default either

ah ok I commented about this below, I'll update it to checking for equality

I think longer run we need a distinction between "broken options" and "developer options". I think this parameter is something that, for now, only developers should be allowed to change, but it isn't necessarily "broken" (especially, as you say, if reducing it below the default).

Aurashk · 2026-02-24T13:57:35Z

Thanks both, I've added @alexdewar suggested changes. Although one thing I thought as I was making the changes is that it makes sense to me to let them change the parameter freely without please_give_me_broken_results as long as they are making it smaller than the default i.e. any value 0 <= remaining_demand_absolute_tolerance <= 1e-12 (default). Can change this though if you think it's better to block any non-default value.

I'l also wondering if we should add more detail for the user in this error so it's clear what's caused the lack of feasible investments without looking at the code. Not sure the best way, maybe we could output all the non-zero unmet demands? On the other hand I suppose you can get the information you need --debug-model option.

tsmbland · 2026-02-24T14:11:44Z

I'l also wondering if we should add more detail for the user in this error so it's clear what's caused the lack of feasible investments without looking at the code. Not sure the best way, maybe we could output all the non-zero unmet demands?

I think this is a good idea.

We should also think about all the scenarios in which this might happen. With NPV now supposedly fixed (we should update the comment to remove this), I think that too restrictive investment constraints and the point in the comment about activity limits are the only ways this can happen (other issues should be flagged at the validation stage, in theory...)

force all non-default remaining_demand_absolute_tolerance parameter changes to require borken results enabled

Aurashk · 2026-02-24T16:12:51Z

I've added more details when no feasible investment options, so for example, with the original issue #1115, the error would now look like the below instead:

[16:04:22 ERROR muse2] Agent investment failed

Caused by:
    No feasible investment options left for commodity 'GASNAT', region 'GBR', year '2034', agent 'A0_GPR' after appraisal.
    Remaining unmet demand (time_slice : flow):
    winter.night : 7.105427357601002e-14

add remaining_demand_absolute_tolerance param

4a0060a

Copilot AI review requested due to automatic review settings February 19, 2026 21:14

Copilot started reviewing on behalf of Aurashk February 19, 2026 21:14 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

add doc for new tol param

3bfb3c0

Aurashk requested a review from alexdewar February 19, 2026 21:32

Merge branch 'main' into add-unmet-demand-threshold

d88d907

alexdewar requested changes Feb 23, 2026

View reviewed changes

tsmbland reviewed Feb 24, 2026

View reviewed changes

guard remaining_demand_absolute_tolerance param behind please_give_br…

4abf55b

…oken_options set units of remaining_demand_absolute_tolerance to Flow

Copilot AI review requested due to automatic review settings February 24, 2026 13:36

Copilot started reviewing on behalf of Aurashk February 24, 2026 13:37 View session

Merge branch 'main' into add-unmet-demand-threshold

5135d29

Copilot AI reviewed Feb 24, 2026

View reviewed changes

tsmbland reviewed Feb 24, 2026

View reviewed changes

add more information when no feasible investment options left

f4026b4

force all non-default remaining_demand_absolute_tolerance parameter changes to require borken results enabled

Aurashk requested a review from alexdewar February 24, 2026 16:12

-    default: 1e-12
+    default: 1e-12
+    notes: |
+      This is a very strict tolerance; the default value is recommended for most cases.
+      The validation currently rejects values larger than the default unless
+      `please_give_me_broken_results` is set to true. Increasing this tolerance may
+      lead to incorrect or misleading investment results and should only be done with
+      a full understanding of the implications.

	// we already checked value is positive, but if they are
	// we already checked value is non-negative, but if they are

Comments

Conversation

Aurashk commented Feb 19, 2026

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

alexdewar Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Aurashk Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Aurashk commented Feb 24, 2026

Uh oh!

tsmbland commented Feb 24, 2026

Uh oh!

Aurashk commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 19, 2026 •

edited

Loading