Skip to content

Fix warning about assets commissioned before simulation start #1259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alexdewar wants to merge 4 commits into
base: main
Choose a base branch
from
+18 −4
Open

Fix warning about assets commissioned before simulation start #1259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
649935a
Fix incorrect lifetime in warning message about old assets
alexdewar Apr 22, 2026
e26e686
Add placeholder sections to release notes doc
alexdewar Apr 22, 2026
2eccf45
Add this bugfix to release notes
alexdewar Apr 22, 2026
9d58c11
Disallow assets where `max_decommission_year == commission_year`
alexdewar Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/release_notes/upcoming.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,17 @@ ready to be released, carry out the following steps:
version

-->

## New features

<!-- TODO -->

## Breaking changes

<!-- TODO -->

## Bug fixes

- Fix misleading warning message for assets decommissioned before simulation start ([#1259])
Comment on lines +23 to +27
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
<!-- TODO -->
## 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 uses AI. Check for mistakes.

[#1259]: https://github.com/EnergySystemsModellingLab/MUSE2/pull/1259
4 changes: 2 additions & 2 deletions src/asset.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,8 @@ impl Asset {
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"
);
Comment on lines 340 to 345
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

Ok(Self {
Expand Down
4 changes: 2 additions & 2 deletions src/asset/pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ impl AssetPool {
// Ignore assets that have already been decommissioned
if asset.max_decommission_year() <= year {
warn!(
"Asset '{}' with commission year {} and lifetime {} was decommissioned before \
"User asset '{}' with commission year {} was decommissioned in {}, before \
the start of the simulation",
asset.process_id(),
Comment on lines +38 to 40
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}".

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Collaborator Author

@alexdewar alexdewar Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

asset.commission_year,
Comment on lines +38 to 41
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
asset.process_parameter.lifetime
asset.max_decommission_year
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copilot uses AI. Check for mistakes.
);
continue;
}
Expand Down
Loading