Fix warning about assets commissioned before simulation start by alexdewar · Pull Request #1259 · EnergySystemsModellingLab/MUSE2

alexdewar · 2026-04-22T15:13:28Z

Description

Users are permitted to define assets in assets.csv that are decommissioned before the simulation starts. If this happens, users are warned, in case it was a mistake, but the warning message reports the process lifetime, which is misleading as max_decommission_year may be set to more or less than this value.

Instead of correcting the lifetime stated in the warning, I thought it was clearer to rewrite the message to include the decommission year directly.

(I just went through this issue with @mjademitchell and @AdrianDAlessandro as a demo.)

Fixes #1220.

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

Copilot

Pull request overview

Updates the warning emitted when a user-defined asset is already decommissioned by the time it would be commissioned, so the warning reports the decommission year (rather than an often-misleading lifetime) per issue #1220.

Changes:

Rewrite the warning message in AssetPool::commission_new to include the asset’s decommission year (max_decommission_year).
Remove the lifetime value from that warning to avoid confusion when max_decommission_year overrides lifetime.

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

Copilot · 2026-04-22T15:17:16Z

+                    "User asset '{}' with commission year {} was decommissioned in {}, before \
                    the start of the simulation",
                    asset.process_id(),


The warning text says "before the start of the simulation", but this branch can also trigger in later milestone years (e.g., an asset with commission_year == year and max_decommission_year == year). In that case the message would be misleading/contradictory. Consider wording it relative to the current milestone year (include year in the message), e.g. "... was decommissioned in {max_decommission_year}, so it is not active in milestone year {year}".

I guess this would happen if the user set commission_year and max_decommission_year for an asset to the same year, which would be a strange thing to do, but we don't ban in - maybe we should?

I guess technically we don't know the year that the asset was decommissioned, as we're not modelling the years before the simulation - we only know the maximum decommission year. So maybe soften this a bit, e.g. "Asset x with maximum decommission year y was decommissioned before the start of the simulation"

I guess this would happen if the user set commission_year and max_decommission_year for an asset to the same year, which would be a strange thing to do, but we don't ban in - maybe we should?

True! I can't think of a sensible reason why a user would want to do this. There may be historical data where assets are commissioned and decommissioned within a year, but these will never have any affect on the MUSE2 simulation, so there's no point in including them. This is different to assets decommissioned before the simulation start, where they also won't have an affect, but could do if you decide to look at a different set of milestone years. So let's just forbid it. I'll do that now.

I guess technically we don't know the year that the asset was decommissioned, as we're not modelling the years before the simulation - we only know the maximum decommission year. So maybe soften this a bit, e.g. "Asset x with maximum decommission year y was decommissioned before the start of the simulation"

The thing is, we kind of do know the year it was decommissioned. Assets commissioned before the simulation start are only counted as decommissioned if max_decommission_year is less than or equal to the first milestone year, so we're treating them as if they can't be decommissioned by agents until the simulation begins. Basically, the semantics of max_decommission_year are slightly different depending on whether it's before or after the simulation start. It's a bit weird, but I think this is the approach we came up with with @martinstringer in #916.

The thing is, we kind of do know the year it was decommissioned. Assets commissioned before the simulation start are only counted as decommissioned if max_decommission_year is less than or equal to the first milestone year, so we're treating them as if they can't be decommissioned by agents until the simulation begins. Basically, the semantics of max_decommission_year are slightly different depending on whether it's before or after the simulation start. It's a bit weird, but I think this is the approach we came up with with @martinstringer in #916.

I was basically saying that, for example, if a simulation starts in 2030 and we have an asset with a max_decommission_year of 2010, we don't know for sure that the asset was definitely decommissioned in 2010 and not before (nor does it matter), just that it can't possibly be around in 2030. But you're saying that if the max_decommission_year is before the start of the simulation then that is the decommission year - right?

I don't really have an issue with this, just that if the only reason to have a slightly complicated definition of max_decommission_year is for the purposes of this message, then maybe we can get around this by softening the language of the message.

Copilot · 2026-04-22T15:17:17Z

                    asset.process_id(),
                    asset.commission_year,
-                    asset.process_parameter.lifetime
+                    asset.max_decommission_year


This uses direct field access (asset.max_decommission_year) while nearby code uses the getter max_decommission_year(). For consistency (and to avoid bypassing any future logic in the accessor), use the method in the warning args as well.

Suggested change

asset.max_decommission_year

asset.max_decommission_year()

Fixes #1220.

codecov · 2026-04-22T15:24:20Z

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (fde0a06) to head (9d58c11).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/asset/pool.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1259   +/-   ##
=======================================
  Coverage   89.74%   89.74%           
=======================================
  Files          57       57           
  Lines        8195     8195           
  Branches     8195     8195           
=======================================
  Hits         7355     7355           
  Misses        544      544           
  Partials      296      296

☔ 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.

tsmbland

Looks good! Just some small nitpicks

tsmbland · 2026-04-22T16:04:56Z

+                    "User asset '{}' with commission year {} was decommissioned in {}, before \
                    the start of the simulation",
                    asset.process_id(),


I guess this would happen if the user set commission_year and max_decommission_year for an asset to the same year, which would be a strange thing to do, but we don't ban in - maybe we should?

tsmbland · 2026-04-22T16:11:34Z

+                    "User asset '{}' with commission year {} was decommissioned in {}, before \
                    the start of the simulation",
                    asset.process_id(),


I guess technically we don't know the year that the asset was decommissioned, as we're not modelling the years before the simulation - we only know the maximum decommission year. So maybe soften this a bit, e.g. "Asset x with maximum decommission year y was decommissioned before the start of the simulation"

Copilot

Pull request overview

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

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

Copilot · 2026-04-23T14:05:35Z

+                    "User asset '{}' with commission year {} was decommissioned in {}, before \
                    the start of the simulation",
                    asset.process_id(),
                    asset.commission_year,


The warning still hard-codes "before the start of the simulation", but commission_new is called for every milestone year. If a user asset has commission_year between milestone years, it can first be considered in a later milestone year and still hit this branch (when max_decommission_year <= year), even though that later year is not the simulation start. Consider wording this relative to the current milestone year (include year in the message) and/or refer to the max decommission year rather than asserting it "was decommissioned in" that year.

Suggested change

"User asset '{}' with commission year {} was decommissioned in {}, before \

the start of the simulation",

asset.process_id(),

asset.commission_year,

"Skipping user asset '{}' with commission year {} in milestone year {} \

because its max decommission year is {}",

asset.process_id(),

asset.commission_year,

year,

Copilot · 2026-04-23T14:05:35Z

        let max_decommission_year =
            max_decommission_year.unwrap_or(commission_year + process_parameter.lifetime);
        ensure!(
-            max_decommission_year >= commission_year,
-            "Max decommission year must be after/same as commission year"
+            max_decommission_year > commission_year,
+            "Max decommission year must be greater than commission year"
        );


This changes validation to reject max_decommission_year == commission_year (previously allowed). Since this is user-facing input validation, it would be good to add a regression test asserting that creating an asset with max_decommission_year equal to commission_year returns an error (and ideally checks the error message), so the intended constraint doesn’t get loosened unintentionally later.

Copilot · 2026-04-23T14:05:36Z

+<!-- TODO -->
+
+## Bug fixes
+
+- Fix misleading warning message for assets decommissioned before simulation start ([#1259])


Release notes mention the warning-message change, but this PR also tightens validation by rejecting assets where max_decommission_year == commission_year. Since that can cause previously accepted inputs to error, consider noting it explicitly here (and potentially under "Breaking changes" if you consider it a breaking input change).

Suggested change



## Bug fixes

- Fix misleading warning message for assets decommissioned before simulation start ([#1259])

- Assets where `max_decommission_year == commission_year` are now rejected during validation. Inputs

that were previously accepted with equal commission and max decommission years may now error

([#1259])

## Bug fixes

- Fix misleading warning message for assets decommissioned before simulation start, and tighten

validation to reject assets where `max_decommission_year == commission_year` ([#1259])

Copilot AI review requested due to automatic review settings April 22, 2026 15:13

Copilot started reviewing on behalf of alexdewar April 22, 2026 15:13 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

alexdewar added 3 commits April 22, 2026 16:17

Fix incorrect lifetime in warning message about old assets

649935a

Fixes #1220.

Add placeholder sections to release notes doc

e26e686

Add this bugfix to release notes

2eccf45

alexdewar force-pushed the fix-old-assets-warning branch from 6de75f8 to 2eccf45 Compare April 22, 2026 15:17

alexdewar requested review from AdrianDAlessandro, Aurashk, mjademitchell and tsmbland April 22, 2026 15:20

tsmbland approved these changes Apr 22, 2026

View reviewed changes

Disallow assets where max_decommission_year == commission_year

9d58c11

Copilot AI review requested due to automatic review settings April 23, 2026 14:00

Copilot started reviewing on behalf of alexdewar April 23, 2026 14:01 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Conversation

alexdewar commented Apr 22, 2026

Description

Type of change

Key checklist

Further checks

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

tsmbland Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Apr 22, 2026

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 Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 22, 2026 •

edited

Loading