Enforce total capacity limit constraint in investment - #1496
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1496 +/- ##
==========================================
+ Coverage 90.35% 90.40% +0.05%
==========================================
Files 61 61
Lines 8850 8899 +49
Branches 8850 8899 +49
==========================================
+ Hits 7996 8045 +49
Misses 529 529
Partials 325 325 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Yes and yes! Sorry, should have alerted you more to #1495 as it was intended to make your life easier not harder! |
…tal_limit_rough # Conflicts: # src/asset.rs # src/simulation/investment.rs # src/simulation/market.rs
There was a problem hiding this comment.
Pull request overview
This PR introduces support for enforcing per-process total installed capacity limits during the investment asset-selection loop, alongside the existing addition-limit behaviour. It extends the input constraint model to carry a total_capacity_limit and threads an “agent share” of that limit through market selection into the investment selection routine.
Changes:
- Add
total_capacity_limittoProcessInvestmentConstraint, including parsing fromprocess_investment_constraints.csvand accessor plumbing (Process::agent_total_limit). - Generalise market-side limit collection via
collect_agent_limitsand pass both addition + total limits intoselect_best_assets. - Extend the investment selection loop to track remaining total capacity per process and filter options accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/simulation/market.rs | Collects per-process agent addition/total limits via a generalised helper and passes both into investment selection. |
| src/simulation/investment.rs | Adds total-limit tracking and applies limit-based filtering during iterative asset selection. |
| src/process.rs | Extends investment constraint model with total_capacity_limit and adds agent_total_limit logic plus unit tests. |
| src/input/process/investment_constraints.rs | Parses total_capacity_limit from CSV into ProcessInvestmentConstraint. |
Suppressed comments (1)
src/simulation/investment.rs:533
remove_assets_exceeding_agent_limitsdoes not apply the limit whenonly_candidatesisfalse: the predicate short-circuits totruefor every asset, so total capacity limits are never enforced.
!(asset.is_candidate() && only_candidates)
|| remaining_agent_limits
.get(asset.process_id())
.is_none_or(|limit| *limit >= asset.total_capacity())
});
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Remove assets that would exceed the remaining limits for their processes from the options | ||
| // The addition limit applies only to candidate assets, the total limit applies to all assets | ||
| remove_assets_exceeding_agent_limits( | ||
| &mut opt_assets, | ||
| &remaining_agent_addition_limits, |
|
@tsmbland This should be ready to review now! It's missing some tests in the investments module. I can add those for the functions I added if you're happy with them, but it's worth noting that the pre-existing code wasn't covered by tests at all |
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Looks good! Only a few small comments.
I wouldn't worry too much about tests, as a lot of the functions either require fiddly data inputs or are too simple to really need tests. You're very welcome to add some though!
Co-authored-by: Tom Bland <t.bland@imperial.ac.uk>
# Conflicts: # benches/assets.rs # src/process.rs
@tsmbland I added a couple tests. Luckily I did, because I found a bug! |
Fair play! I had to stare at that line for several minutes to figure out the problem, but indeed looks like it was a bug. Shame on copilot review for not picking this up. |
Description
This is an attempt at enforcing the total capacity limit constraint in the investment. It still needs some tests, but there are some components I wanted to ask about first.
I had made the total capacity limits mapped by
ProcessID, but was going to ask about the function in which I do that (market.rs::collect_total_limits), because I believe it is looping over duplicate processes and just overwriting theHashMapunnecessarily. However, I have just seen #1495 - which I believe answers that question, but will require me to bring this branch up to date with it and then change my implementation.For now, two questions:
Fixes #1493
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks