Validate flows for non-primary outputs/inputs by dalonsoa · Pull Request #962 · EnergySystemsModellingLab/MUSE2

dalonsoa · 2025-10-22T08:51:11Z

Description

Adds a validation for the non-primary flows of a process, which must exist for all years and be of the same sign, i.e. always inputs or always outputs in all years. They can differ in different regions, though.

TODO

I've checked that this works as expected by playing around with the two_outputs example. It catches the situations described and report the right error. However, I am unable to write a test for it, fighting against borrowing and mutability rules of rust. Any advice on this front, @tsmbland , would be most welcomed.

So far, what I've tried is to copy one of the other tests on the topic and manually change the flow of one specific year for one specific region and commodity in the flow_map before passing it to the validation function, but that does not seem to be permitted...

Fixes #927
Fixes #929

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-10-22T08:53:09Z

Codecov Report

❌ Patch coverage is 78.78788% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.85%. Comparing base (07f1f53) to head (03f3cfd).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
src/input/process/flow.rs	78.78%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #962      +/-   ##
==========================================
- Coverage   83.86%   83.85%   -0.02%     
==========================================
  Files          50       50              
  Lines        5541     5573      +32     
  Branches     5541     5573      +32     
==========================================
+ Hits         4647     4673      +26     
- Misses        658      662       +4     
- Partials      236      238       +2

☔ 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 and nicely kills two birds with one stone.

Worth noting that this will have to change slightly with #928. In this case we'll have inputs, outputs and zeros, so I don't think the Vec<bool> approach would work. But off the top of my head I can't think of an easy solution...

I wouldn't worry too much about tests for now. This sort of thing will be much easier if/when #655 is done

dalonsoa · 2025-10-22T14:17:35Z

We will adapt this when needed. I think it would not be too much trouble.

If tests are not something to worry about at the moment, then this is ready to go.

alexdewar

Some minor suggestions, but totally up to you if you want to do them.

In terms of the bigger picture, this code has the same problem that the existing flows validation code has in that it assumes that users need to provide entries for every year in Process::years (see #934). (I'm not saying you should change this now, but I just wanted to make you aware of it!) We probably only want to require entries for the milestone years though (processes can have a start_year before the start of the simulation and in the longer run, they could be commissioned in any year: #859). The non-milestone years should be checked lazily (i.e. when we commission an asset). That said, it probably makes sense to check that all the process flows entries that are actually specified are of the correct type (input/output) in any case. Anyway, we can worry about this when we do #934.

alexdewar · 2025-10-23T10:32:58Z

src/input/process/flow.rs

            ensure!(
-                (0.0..f64::INFINITY).contains(&cost.value()),
+                (cost.value() >= 0.0),
                "Invalid value for flow cost ({cost}). Must be >=0."


It was weird the way it was written before! I guess the previous check also verified that it was finite, but that's not a worry since we do that check when we deserialise now anyway

alexdewar · 2025-10-23T10:33:48Z

src/input/process/flow.rs


+/// Checks that non-primary io are defined for all years (within a region) and that
+/// they are only inputs or only outputs in all years.
+fn validate_secondary_io_flows(


I'd probably just call this validate_secondary_flows

alexdewar · 2025-10-23T10:36:51Z

src/input/process/flow.rs

+        // Get the flows for this process - there should be no error, as was checked already
+        let map = flows_map
+            .get(process_id)
+            .with_context(|| format!("Missing flows map for process {process_id}"))?;


Given that this function and validate_flows_and_primary_output both iterate over the process maps, we could have that loop outside the functions instead and just pass in a Process and ProcessFlowsMap to each. Might be cleaner.

Not particularly important though.

I thought about it, but I did not want to intervene the existing functionality too much. Also, being used just during loading of data, didn't consider performance to be an issue.

I was thinking more in terms of tidiness than performance, but I agree that it's ok as it is!

alexdewar · 2025-10-23T10:50:45Z

src/input/process/flow.rs

+
+        // Get the non-primary io flows for all years, if any, arranged by (commodity, region)
+        let iter = iproduct!(process.years.iter(), process.regions.iter());
+        let mut flows: HashMap<(CommodityID, RegionID), Vec<bool>> = HashMap::new();


I think this code could be simpler if you just included the year in the key, i.e. made this a HashMap<(CommodityID, RegionID, u32), bool>.

We do not need the year for anything, so why storing that information at all? If I do that, it will also be harder to check if all the values for a specific commodity and region combination are of the same sign and complete.

Actually, I'm not quite sure what I meant here... it is fine the way it is.

dalonsoa

The issue of the years needs to be address globaly in multiple places. This just follows the same format than the primary output validation for consistency. I feel it will be wrong to do something different here.

dalonsoa · 2025-10-24T04:35:15Z

src/input/process/flow.rs

+
+        // Get the non-primary io flows for all years, if any, arranged by (commodity, region)
+        let iter = iproduct!(process.years.iter(), process.regions.iter());
+        let mut flows: HashMap<(CommodityID, RegionID), Vec<bool>> = HashMap::new();


We do not need the year for anything, so why storing that information at all? If I do that, it will also be harder to check if all the values for a specific commodity and region combination are of the same sign and complete.

alexdewar · 2025-10-24T11:19:41Z

The issue of the years needs to be address globaly in multiple places. This just follows the same format than the primary output validation for consistency. I feel it will be wrong to do something different here.

Yep, it will need fixing globally. I wasn't suggesting you redo it now, just flagging that it will need revisiting.

dalonsoa added 2 commits October 21, 2025 10:56

♻️ Change comparison in costs

27702fe

✨ Add validation of secondary flows

778d626

dalonsoa requested review from Aurashk, alexdewar and tsmbland and removed request for tsmbland October 22, 2025 08:51

Fix clippy complains

36de838

tsmbland approved these changes Oct 22, 2025

View reviewed changes

Merge branch 'main' into 927_flows

deb6d57

alexdewar approved these changes Oct 23, 2025

View reviewed changes

dalonsoa commented Oct 24, 2025

View reviewed changes

Rename validate_secondary_io_flows to validate_secondary_flows

03f3cfd

dalonsoa merged commit 4fcea11 into main Oct 24, 2025
8 checks passed

dalonsoa deleted the 927_flows branch October 24, 2025 12:10

Conversation

dalonsoa commented Oct 22, 2025

Description

TODO

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Oct 22, 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!

dalonsoa commented Oct 22, 2025

Uh oh!

alexdewar 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexdewar commented Oct 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 22, 2025 •

edited

Loading