Skip to content

Clean up how we specify default values for model parameters#1260

Merged
alexdewar merged 2 commits intomainfrom
cleaner-model.toml-defaults
Apr 28, 2026
Merged

Clean up how we specify default values for model parameters#1260
alexdewar merged 2 commits intomainfrom
cleaner-model.toml-defaults

Conversation

@alexdewar
Copy link
Copy Markdown
Member

@alexdewar alexdewar commented Apr 23, 2026

Description

I started work on #420, but thought the code could use a little clean-up first, so I've opened this as a separate PR.

The way we currently define default values for model parameters (as read from model.toml) is with a couple of macros in src/model/parameters.rs, but this ends up being a bit verbose and ugly. I've cleaned it up by using a custom Default implementation for the ModelParameters struct instead, which lets us list the default values in a more natural way. The downside of this approach is that it does mean we need to define a "default" value for milestone_years, even though this parameter is not optional, then verify that a value was provided after the fact. I think it's worth it overall though.

While I was at it I changed the capacity_margin parameter to be a Dimensionless rather than an f64, to make it consistent with the other parameters. It's good to be explicit that it represents a ratio, not some physical value.

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

Let's make it consistent with other parameters. This way is less ambiguous.
Copilot AI review requested due to automatic review settings April 23, 2026 13:39
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.76%. Comparing base (a10a1f4) to head (6684455).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1260      +/-   ##
==========================================
+ Coverage   89.74%   89.76%   +0.01%     
==========================================
  Files          57       57              
  Lines        8195     8210      +15     
  Branches     8195     8210      +15     
==========================================
+ Hits         7355     7370      +15     
  Misses        544      544              
  Partials      296      296              

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors how model.toml parameter defaults are defined by moving from per-field default macros to a Default implementation on ModelParameters, and aligns capacity_margin with the codebase’s unit-typed parameters (Dimensionless) for consistency.

Changes:

  • Replace macro-based per-field defaults in ModelParameters with #[serde(default)] + a custom Default impl, while validating required milestone_years post-deserialization.
  • Change capacity_margin from f64 to Dimensionless in model parameters and propagate the type change into dispatch optimisation.

Reviewed changes

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

File Description
src/simulation/optimisation.rs Updates DispatchRun/capacity variable plumbing to accept Dimensionless capacity_margin, converting to f64 only at the solver-boundary.
src/model/parameters.rs Introduces ModelParameters: Default to centralize defaults, removes old default macros, improves the milestone_years validation message, and makes capacity_margin unit-typed.

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

@alexdewar alexdewar added this to MUSE Apr 23, 2026
@alexdewar alexdewar moved this to 👀 In review in MUSE Apr 23, 2026
@alexdewar alexdewar force-pushed the cleaner-model.toml-defaults branch from 6a006c0 to 6684455 Compare April 23, 2026 14:05
Copy link
Copy Markdown
Collaborator

@Aurashk Aurashk left a comment

Choose a reason for hiding this comment

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

Looks like an improvement to me

@alexdewar
Copy link
Copy Markdown
Member Author

Thanks @Aurashk. I'm going to merge this so I can open a PR for #420.

@alexdewar alexdewar merged commit 1c34ca0 into main Apr 28, 2026
8 checks passed
@alexdewar alexdewar deleted the cleaner-model.toml-defaults branch April 28, 2026 18:45
@github-project-automation github-project-automation Bot moved this from 👀 In review to ✅ Done in MUSE Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants