Make `Process` commission year a `RangeInclusive` by dalonsoa · Pull Request #1010 · EnergySystemsModellingLab/MUSE2

dalonsoa · 2025-11-21T14:40:24Z

Description

This is the first of a series of modifications aimed at refactoring how process maps are represented and used. See #880 for more details on the purpose of this refactoring.

This particular PR changes the code so the process valid commission years are stored as a RangeInclusive instead of a discrete vector of years. The direct consequence of this is that input assets can be commissioned in any year where the process can be commissioned, and not just the milestone years or years before the base year.

As a consequence of this change, the following also had to be made:

process parameters now are validated only in the milestone years within the process commission years (as done for flows in Check flows in milestone years only #995)
the validation of secondary flows also need to be refactored to use this range instead, fixing a bug along the way.

Finally, a validation step has been added when reading assets where the commission year is checked against the range of valid commission years and the years available in the parameters and flows.

No change has been made to how the parameters and flow maps are handled (suggested to use rangemap) as that requires significant refactoring and needs to be well thought.

Fixes #859
Fixes #770

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

Further checks

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

codecov · 2025-11-21T14:42:20Z

Codecov Report

❌ Patch coverage is 73.84615% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.99%. Comparing base (8f1a2eb) to head (fa927e5).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/input/process.rs	46.66%	5 Missing and 3 partials ⚠️
src/input/asset.rs	60.00%	4 Missing ⚠️
src/input/process/availability.rs	75.00%	0 Missing and 3 partials ⚠️
src/input/process/flow.rs	92.85%	0 Missing and 1 partial ⚠️
src/input/process/parameter.rs	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1010      +/-   ##
==========================================
- Coverage   81.13%   80.99%   -0.14%     
==========================================
  Files          52       52              
  Lines        6287     6299      +12     
  Branches     6287     6299      +12     
==========================================
+ Hits         5101     5102       +1     
- Misses        932      942      +10     
- Partials      254      255       +1

☔ 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

I think this seems like a cleaner approach overall

There will be a performance cost to this, because now any time you provide "all" as the year, you're going to end up cloning the data over all years in the process range, whereas before it would just be the milestone years. I don't know how significant this will be.

This is one reason to go ahead with #874, but as you've mentioned it's debatable whether this is worth it

tsmbland · 2025-11-21T15:52:00Z

src/input/process/availability.rs

        let record_years =
-            parse_year_str(&record.commission_years, process_years).with_context(|| {
+            parse_year_str(&record.commission_years, &process_years).with_context(|| {
                format!("Invalid year for process {id}. Valid years are {process_years:?}")


Probably fine, although this could be a lot of years which will make for a long message. Maybe better to give the range of years rather than the full list?

I also wonder if parse_year_str could be simplified by taking a range instead of a list of years?

I thought about this, but parse_year_str is used in other places passing the milestone years as valid years, which is a discrete vector. In python handling this two possible inputs would be a piece of cake, but I don't see a clear way forward here but by duplicating the function, one taking a vector as input and another a range, updating the internal logic internally as needed.

Hmm good point. I'd say it's best as it is then

tsmbland · 2025-11-21T15:55:23Z

src/input/process/availability.rs

-        .iter()
-        .copied()
-        .filter(|&year| year >= base_year);
+    let process_milestone_years = process.years.clone().filter(|&year| year >= base_year);


I think this is wrong. We need to filter this to just include milestone years. Currently it includes all years after the first milestone year

You'll need to pass the list of milestone years into the function to do this

tsmbland · 2025-11-21T16:04:31Z

src/input/asset.rs

+            asset.process_id,
+            asset.commission_year,
+            asset.agent_id,
+        );


I like this block a lot, but aren't we already doing these checks elsewhere?

Yes, sort of, when building the commodity graph for the model. I just thought it made sense to catch issues even earlier, when processing the file. Happy to remove it if you think it is redundant, but I don't think it does any harm.

tsmbland · 2025-11-21T16:07:56Z

src/input/process.rs

-                .filter(|year| (start_year..=end_year).contains(year)),
-        )
-        .collect();
+        // simulation's time horizon, so assume that all years >=start_year are valid too.


I don't think we need this comment any more

dalonsoa · 2025-11-24T09:18:51Z

@tsmbland Wait to re-review. I've made the changes to the wrong branch... :(

dalonsoa · 2025-11-24T09:24:08Z

Done, now.

tsmbland · 2025-11-24T09:43:49Z

src/input/process/availability.rs

-    let process_milestone_years = process.years.clone().filter(|&year| year >= base_year);
    let mut missing = Vec::new();
-    for (region_id, year) in iproduct!(&process.regions, process_milestone_years) {
+    for (region_id, &year) in iproduct!(&process.regions, milestone_years) {


I think this still isn't right. This needs to be just the milestone years within the year range of the process

Again... indeed!

dalonsoa · 2025-11-24T11:37:26Z

Now. Hopefully

tsmbland

Looks good now!

dalonsoa added 2 commits November 21, 2025 13:56

✨ Make process comission years a range

014bdad

Validate comission year of input assets.

53bb216

dalonsoa requested review from Aurashk and tsmbland and removed request for tsmbland November 21, 2025 14:40

Merge branch 'main' into 874_rangemap_year

695912f

tsmbland requested changes Nov 21, 2025

View reviewed changes

dalonsoa requested a review from tsmbland November 24, 2025 09:17

dalonsoa and others added 3 commits November 24, 2025 09:19

Merge branch 'main' into 874_rangemap_year

7c60f55

Use milestone years to validate availability

9929945

Warn when using default years for process

0ca440b

tsmbland reviewed Nov 24, 2025

View reviewed changes

Fix years for availabilities

55fe6aa

tsmbland self-requested a review November 24, 2025 11:54

tsmbland approved these changes Nov 24, 2025

View reviewed changes

Merge branch 'main' into 874_rangemap_year

fa927e5

dalonsoa merged commit 1ba1473 into main Nov 24, 2025
8 checks passed

dalonsoa deleted the 874_rangemap_year branch November 24, 2025 12:30

Conversation

dalonsoa commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Nov 21, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalonsoa commented Nov 24, 2025

Uh oh!

dalonsoa commented Nov 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalonsoa commented Nov 24, 2025

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dalonsoa commented Nov 21, 2025 •

edited

Loading

codecov bot commented Nov 21, 2025 •

edited

Loading