Skip to content

Disallow negative process parameters - #1485

Open
AdrianDAlessandro wants to merge 1 commit into
mainfrom
non-negative-process-parameters
Open

Disallow negative process parameters#1485
AdrianDAlessandro wants to merge 1 commit into
mainfrom
non-negative-process-parameters

Conversation

@AdrianDAlessandro

Copy link
Copy Markdown
Collaborator

Description

This is the simplest way to address this issue. Although it doesn't give the clearest error message:

[17:19:46 INFO muse2::cli] Starting MUSE2 v2.1.0
[17:19:46 ERROR muse2] Failed to load model.

Caused by:
    0: Error reading /private/var/folders/td/5nhj1569101bqjj_vmltr_7r0000gp/T/.tmppPT7Gx/simple/process_parameters.csv
    1: CSV deserialize error: record 1 (line: 2, byte: 117): Value must be a finite number greater than or equal to zero

Fixes #1438

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

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.91%. Comparing base (1c0b2ba) to head (cadedad).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1485   +/-   ##
=======================================
  Coverage   89.91%   89.91%           
=======================================
  Files          60       60           
  Lines        8556     8556           
  Branches     8556     8556           
=======================================
  Hits         7693     7693           
  Misses        543      543           
  Partials      320      320           

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens input validation for process_parameters.csv by preventing negative values from being deserialised into key cost-related process parameters, addressing issue #1438.

Changes:

  • Applies deserialise_finite_non_negative to capital_cost, fixed_operating_cost, and variable_operating_cost during CSV deserialisation.
  • Updates imports in the process-parameter CSV reader to use the shared non-negative finite-number deserialiser.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +24 to 25
#[serde(deserialize_with = "deserialise_finite_non_negative")]
capital_cost: MoneyPerCapacity,
Comment on lines +26 to 29
#[serde(deserialize_with = "deserialise_finite_non_negative")]
fixed_operating_cost: MoneyPerCapacityPerYear,
#[serde(deserialize_with = "deserialise_finite_non_negative")]
variable_operating_cost: MoneyPerActivity,

@tsmbland tsmbland left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is fine for now. Might be worth looking into ways of improving the error message as this could probably help across the board

@AdrianDAlessandro

Copy link
Copy Markdown
Collaborator Author

The lack of clarity in the error output is consistent with other times this custom deserialisation is used in the code.

One thing worth noting, is that when it is used with a toml input file, the error message is quite clear. For example

[16:53:05 ERROR muse2] Failed to load model.

Caused by:
    0: Error reading /private/var/folders/td/5nhj1569101bqjj_vmltr_7r0000gp/T/.tmpaxxQee/circularity/model.toml
    1: TOML parse error at line 2, column 19
         |
       2 | capacity_margin = -0.3
         |                   ^^^^
       Value must be a finite number greater than or equal to zero

But it's less clear with CSV input files

[16:53:19 ERROR muse2] Failed to load model.

Caused by:
    0: Error reading /private/var/folders/td/5nhj1569101bqjj_vmltr_7r0000gp/T/.tmpzLMuo8/circularity/agent_commodity_portions.csv
    1: CSV deserialize error: record 11 (line: 12, byte: 246): Value must be > 0 and <= 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Input validation does not flag negative capital/fixed/variable costs

3 participants