Skip to content

Rip out code for old optimisation model#595

Merged
alexdewar merged 7 commits intomainfrom
prepare-for-new-model
Jun 3, 2025
Merged

Rip out code for old optimisation model#595
alexdewar merged 7 commits intomainfrom
prepare-for-new-model

Conversation

@alexdewar
Copy link
Copy Markdown
Member

Description

I've ripped out the code for the old model, mostly following my proposal in #576. Some things might well turn out to be easy to implement, but I figured it was probably best to just start from zero here rather than spending time trying to get new bits of code working before opening a PR. I decided to delete code rather than just commenting it out, to make the diffs cleaner when we add the new implementations in.

I've held off on changing the code for getting commodity flows, although that was trivial enough. The reason is because I'm thinking it might be better to change the code to represent assets mostly with Rc<Asset>s rather than AssetIDs (#591), which, among other things, would mean we don't have to pass around a reference to the AssetPool to various functions. I've started work on that already as it made sense for #483, so I can open a PR for that soon -- provided that you don't think it's a stupid idea!

I decided to disable the regression test for now to avoid unnecessary churn while we add all the parts for the new model. We should re-enable it once we're done (#594).

Closes #576.

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

@alexdewar alexdewar requested review from Copilot and tsmbland June 2, 2025 14:31
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.51%. Comparing base (6432176) to head (dc33f06).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/simulation/optimisation.rs 81.25% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #595      +/-   ##
==========================================
- Coverage   89.47%   84.51%   -4.96%     
==========================================
  Files          37       37              
  Lines        3544     3255     -289     
  Branches     3544     3255     -289     
==========================================
- Hits         3171     2751     -420     
- Misses        179      318     +139     
+ Partials      194      186       -8     

☔ 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

This PR removes the legacy optimisation model code to prepare for a fresh implementation and disables the associated regression test.

  • Disabled the existing regression test
  • Deleted old CSV debug/output files
  • Stubbed out optimisation, pricing, and constraint code with TODO placeholders

Reviewed Changes

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

Show a summary per file
File Description
tests/regression_simple.rs Marked regression test as ignored while refactoring
tests/data/simple/*.csv Removed all old debug CSV files for the simple regression model
src/simulation/prices.rs Removed detailed price‐calculation logic and added stub TODO
src/simulation/optimisation/constraints.rs Deleted constraint implementations and added stub TODOs
src/simulation/optimisation.rs Simplified variable map and cost coefficient stub with TODO
src/output.rs Stripped out fixed–asset dual writer code
Comments suppressed due to low confidence (5)

tests/regression_simple.rs:9

  • Ignoring the regression test reduces coverage of the optimization model; ensure this is tracked and add replacement tests or re-enable it once the new implementation is in place.
#[ignore]

src/simulation/prices.rs:58

  • The price‐calculation method is stubbed and always returns an empty set, which will break any logic depending on commodity prices; implement the correct algorithm before dispatch.
// **TODO:** Calculate commodity prices here:

src/simulation/optimisation/constraints.rs:98

  • Commodity balance constraints are not implemented, leaving the optimization model under‐constrained; add these constraints to enforce supply/demand balance.
// **TODO:** Add commodity balance constraints:

src/simulation/optimisation/constraints.rs:124

  • Capacity and availability constraints are stubbed out, allowing unlimited asset activity; restore proper limit logic to prevent invalid optimization results.
// **TODO:** Add capacity/availability constraints:

src/simulation/optimisation.rs:200

  • Cost coefficient is hard‐coded to 1.0, which misrepresents asset costs in the objective function; reimplement accurate cost computations to ensure valid optimization outcomes.
// **TODO:** Calculate cost coefficient here:

Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland left a comment

Choose a reason for hiding this comment

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

😢

Comment thread src/simulation/optimisation.rs Outdated
for time_slice in model.time_slice_info.iter_ids() {
let coeff = calculate_cost_coefficient(asset, year, time_slice);

// Question: Might activity be negative? If so, the bounds will need to be inverted
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.

Nah definitely won't have negative activity

@alexdewar alexdewar enabled auto-merge June 3, 2025 11:39
@alexdewar alexdewar merged commit d7fc05f into main Jun 3, 2025
7 checks passed
@alexdewar alexdewar deleted the prepare-for-new-model branch June 3, 2025 11:41
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.

Prepare code for implementing new optimisation model

3 participants