Smarter asset grouping - #1492
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## asset_equalisation #1492 +/- ##
======================================================
- Coverage 90.04% 90.01% -0.04%
======================================================
Files 60 60
Lines 8597 8659 +62
Branches 8597 8659 +62
======================================================
+ Hits 7741 7794 +53
- Misses 537 545 +8
- Partials 319 320 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refines the “equal utilisation” dispatch constraints by introducing a more robust notion of dispatch-equivalence between assets, so the optimiser equalises utilisation across assets that behave the same in dispatch (even across different processes), while avoiding equalisation when key dispatch-driving parameters differ.
Changes:
- Update equal-utilisation constraints to group assets by dispatch-equivalence rather than
(region, process)identity. - Introduce
Asset::is_dispatch_equivalentto centralise the equivalence definition (and add unit tests for it).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/simulation/optimisation/constraints.rs | Switch asset grouping strategy for equal-utilisation constraints to use dispatch-equivalence comparisons. |
| src/asset.rs | Add is_dispatch_equivalent helper and tests to define/validate dispatch-equivalence semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/asset.rs:721
is_dispatch_equivalentrelies on==forActivityLimitsand theIndexMapof flows. Both are order-sensitive (because they contain/areIndexMaps), so two assets with identical limits/flows but inserted in a different order (e.g. different CSV row ordering, orActivityLimits::new_from_limitsiterating aHashMapfor seasonal limits) will be treated as not dispatch-equivalent. That undermines the goal of grouping “equivalent” assets across processes/years.
Consider making the comparisons order-insensitive (while keeping the existing Arc::ptr_eq fast path).
pub fn is_dispatch_equivalent(&self, other: &Self) -> bool {
self.region_id == other.region_id
&& (Arc::ptr_eq(&self.activity_limits, &other.activity_limits)
|| self.activity_limits == other.activity_limits)
&& (Arc::ptr_eq(&self.flows, &other.flows) || self.flows == other.flows)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/asset.rs:683
- Doc comment grammar: "Equal hashes does not confirm equivalence" should be "Equal hashes do not confirm equivalence".
/// [`Self::is_dispatch_equivalent`]. Equal hashes does not confirm equivalence, but unequal
Description
#1477 equalises utilisation of equivalent assets in dispatch, to avoid the solver arbitrarily utilising one over another. There's some ambiguity over what "equivalent" means. For the purposes of that PR, equivalent assets are assets of the same process in the same region. However, that isn't particularly robust:
This PR uses a slightly more robust approach of comparing the relevant parameters between assets. Assets in the same region are considered identical if their variable operating costs, flows and availability limits are identical. Comparisons are deliberately conservative such that any difference in any of these, however small, means the assets are considered not equivalent. Adding any kind of tolerance would be much more fiddly. We shouldn't have to worry about floating point differences here as the parameters we're comparing come (almost) directly from the input data.
To avoid lots of potentially expensive pairwise comparisons, the approach is to first create a hash for each asset using the relevant parameters, then only do full comparisons on assets with the same hash (as comparing based on hash alone may create false positives, however unlikely).
Overall, compared with the parent branch, this doesn't seem to have any detectable impact on performance in the models I've tested. Nor does it change any of the example model results, as none of these suffer from process parameters changing over time or redundant processes.
Fixes # (issue)
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks