Don't create unmet demand file unless needed. by dalonsoa · Pull Request #1405 · EnergySystemsModellingLab/MUSE2

dalonsoa · 2026-07-09T13:36:59Z

Description

The path for the file is added to the structure, keeping the actual writer as an option and only created if needed. A check is added to verify that the writer does not exist, which would mean that the run did not stopped after finding unmet demand. The file is closed alongside all the other files, if needed. The regression tests files for the simple case has been amended to removed the empty file, not needed anymore.

Fixes #1372

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-07-09T13:38:59Z

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.45%. Comparing base (18b9bc5) to head (e0d0af5).

Files with missing lines	Patch %	Lines
src/output.rs	83.33%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1405      +/-   ##
==========================================
+ Coverage   89.44%   89.45%   +0.01%     
==========================================
  Files          59       59              
  Lines        8364     8377      +13     
  Branches     8364     8377      +13     
==========================================
+ Hits         7481     7494      +13     
  Misses        562      562              
  Partials      321      321

☔ View full report in Codecov by Harness.
📢 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 one small comment

tsmbland · 2026-07-10T10:24:50Z

+        // If the unmet demand writer already exist, we return with an error, as it should not happen
+        ensure!(
+            self.unmet_demand_writer.is_none(),
+            "Unmet demand file already exists!"


If it's not possible for a user to trigger this, it should panic rather than returning an error

Copilot

Pull request overview

This PR adjusts debug output generation so debug_unmet_demand.csv is only created when unmet demand is actually present, avoiding an always-empty debug file in normal successful runs.

Changes:

Refactors DebugDataWriter to store the unmet-demand file path and make the unmet-demand CSV writer optional (lazy creation).
Updates unmet-demand writing logic to no-op when there are no rows, and updates flushing to handle the optional writer.
Updates regression test data for the simple case by removing the previously committed empty debug_unmet_demand.csv.

Reviewed changes

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

File	Description
src/output.rs	Lazily creates and conditionally flushes the unmet-demand debug CSV writer to avoid producing an empty file.
tests/data/simple/debug_unmet_demand.csv	Removes the committed empty debug output file from regression test data.

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

tsmbland · 2026-07-13T08:34:36Z

+        let rows: Vec<(&CommodityID, &RegionID, &TimeSliceID, Flow)> =
+            iter.collect::<Vec<(&CommodityID, &RegionID, &TimeSliceID, Flow)>>();
+
+        if rows.is_empty() {
+            return Ok(());
+        }
+
+        // If the unmet demand writer already exist, we return with an error, as it should not happen
+        if self.unmet_demand_writer.is_some() {
+            panic!("Unmet demand file already exists!");
+        }
+
+        self.unmet_demand_writer = Some(csv::Writer::from_path(&self.unmet_demand_file_path)?);
+        for (commodity_id, region_id, time_slice, value) in rows {


Copilot has a point, but this can be simpler:

let mut rows = iter.peekable(); if rows.peek().is_none() { return Ok(()); }

Then the rest is unchanged from what you've done

        }

        Ok(())


tsmbland · 2026-07-13T08:53:56Z

-            self.unmet_demand_writer.serialize(row)?;
+            self.unmet_demand_writer
+                .as_mut()
+                .expect("Writer does not exist (cannot not happen)")


Agree that this is a typo (although my brain can't handle the triple negative!).

I think this whole block can be tidied though to avoid having to unwrap here

let run_description = self.with_context(run_description); let writer = self .unmet_demand_writer .insert(csv::Writer::from_path(&self.unmet_demand_file_path)?); for (commodity_id, region_id, time_slice, value) in rows { let row = UnmetDemandRow { milestone_year, run_description: run_description.clone(), commodity_id: commodity_id.clone(), region_id: region_id.clone(), time_slice: time_slice.clone(), value, }; writer.serialize(row)?; }

Replaced panic with assert to handle existing unmet demand writer.

dc2917

Seems logical to me!

dalonsoa · 2026-07-13T08:05:28Z

Copilot's comment seem to make sense to me, but I'm not sure if it is worth investing more time into this, considering that is a very niche use case and that the function will only ever be called once, so performance is not an issue. Any comment @tsmbland @dc2917

dc2917 · 2026-07-13T08:24:58Z

Copilot's comment seem to make sense

Agree

but I'm not sure if it is worth investing more time into this, considering that is a very niche use case and that the function will only ever be called once

Also agree!

I don't have a sense of how large the "large allocation" could be, but rows has a short lifetime - if it is empty, we return from the function and it immediately goes out of scope, so the large allocation will be freed up.

tsmbland

On second look I think there are some things that could be improved. Not so much for performance, mostly just to make the code a bit cleaner

tsmbland · 2026-07-13T08:34:36Z

+        let rows: Vec<(&CommodityID, &RegionID, &TimeSliceID, Flow)> =
+            iter.collect::<Vec<(&CommodityID, &RegionID, &TimeSliceID, Flow)>>();
+
+        if rows.is_empty() {
+            return Ok(());
+        }
+
+        // If the unmet demand writer already exist, we return with an error, as it should not happen
+        if self.unmet_demand_writer.is_some() {
+            panic!("Unmet demand file already exists!");
+        }
+
+        self.unmet_demand_writer = Some(csv::Writer::from_path(&self.unmet_demand_file_path)?);
+        for (commodity_id, region_id, time_slice, value) in rows {


Copilot has a point, but this can be simpler:

let mut rows = iter.peekable(); if rows.peek().is_none() { return Ok(()); }

Then the rest is unchanged from what you've done

tsmbland · 2026-07-13T08:39:53Z

+            return Ok(());
+        }
+
+        // If the unmet demand writer already exist, we return with an error, as it should not happen


Comment needs updating

tsmbland · 2026-07-13T08:53:56Z

-            self.unmet_demand_writer.serialize(row)?;
+            self.unmet_demand_writer
+                .as_mut()
+                .expect("Writer does not exist (cannot not happen)")


Agree that this is a typo (although my brain can't handle the triple negative!).

I think this whole block can be tidied though to avoid having to unwrap here

let run_description = self.with_context(run_description); let writer = self .unmet_demand_writer .insert(csv::Writer::from_path(&self.unmet_demand_file_path)?); for (commodity_id, region_id, time_slice, value) in rows { let row = UnmetDemandRow { milestone_year, run_description: run_description.clone(), commodity_id: commodity_id.clone(), region_id: region_id.clone(), time_slice: time_slice.clone(), value, }; writer.serialize(row)?; }

dalonsoa · 2026-07-14T04:35:48Z

Done! Didn't know about peek, and it's pretty handy!

tsmbland

LGTM!

Don't create unmet demand file unless needed.

e4aa0de

dalonsoa requested review from Copilot, dc2917 and tsmbland and removed request for Copilot July 9, 2026 13:37

Copilot started reviewing on behalf of dalonsoa July 9, 2026 13:37 View session

tsmbland approved these changes Jul 10, 2026

View reviewed changes

Change error handling to panic for unmet demand writer

75a05f3

Copilot AI review requested due to automatic review settings July 10, 2026 10:58

Copilot started reviewing on behalf of dalonsoa July 10, 2026 10:58 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

dalonsoa and others added 3 commits July 10, 2026 12:02

Change panic to assert for unmet demand writer check

95f7ba7

Replaced panic with assert to handle existing unmet demand writer.

[pre-commit.ci lite] apply automatic fixes

4724b99

Merge branch 'main' into 1372_unmet_demand_file

98cdc54

dc2917 approved these changes Jul 13, 2026

View reviewed changes

tsmbland reviewed Jul 13, 2026

View reviewed changes

dalonsoa and others added 2 commits July 14, 2026 05:30

Simplify code

4af24d4

Merge branch 'main' into 1372_unmet_demand_file

e0d0af5

tsmbland approved these changes Jul 14, 2026

View reviewed changes

tsmbland merged commit 48337a3 into main Jul 14, 2026
7 of 8 checks passed

tsmbland deleted the 1372_unmet_demand_file branch July 14, 2026 09:05

Uh oh!

Conversation

dalonsoa commented Jul 9, 2026

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov Bot commented Jul 9, 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 Jul 10, 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

Reviewed changes

Uh oh!

tsmbland Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dc2917 left a comment

Choose a reason for hiding this comment

Uh oh!

dalonsoa commented Jul 13, 2026

Uh oh!

dc2917 commented Jul 13, 2026

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

tsmbland Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalonsoa commented Jul 14, 2026

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!

4 participants

codecov Bot commented Jul 9, 2026 •

edited

Loading

tsmbland Jul 13, 2026 •

edited

Loading

tsmbland Jul 13, 2026 •

edited

Loading