Fix bug with select_assets_for_cycle#1323
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
=======================================
Coverage 89.51% 89.52%
=======================================
Files 58 58
Lines 8537 8542 +5
Branches 8537 8542 +5
=======================================
+ Hits 7642 7647 +5
Misses 580 580
Partials 315 315 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes capacity-limit calculation in select_assets_for_cycle by using the commodity portion associated with each selected asset’s owning agent and its own primary output commodity, rather than the “current” commodity being iterated in the cycle. This prevents incorrect scaling (and potential panics) when the cycle spans multiple commodities owned by different agents.
Changes:
- Compute flexible-capacity installable limits using
(asset.agent_id, asset.primary_output_commodity, year)to look up the correctcommodity_portionsentry. - Add
Asset::primary_output_commodity()helper to access the process’ configured primary output commodity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/simulation/investment.rs |
Updates cycle dispatch flexible-capacity limits to use per-asset agent/commodity portions (fixing incorrect scaling across multi-commodity cycles). |
src/asset.rs |
Adds a small accessor for an asset’s primary output commodity to support the updated investment logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let agent_id = asset.agent_id().unwrap(); | ||
| let commodity_id = asset.primary_output_commodity().unwrap(); | ||
| let agent_share = *agent_share_cache | ||
| .entry(agent_id.clone()) | ||
| .or_insert_with(|| model.agents[agent_id].commodity_portions[&key]); | ||
| .entry((agent_id, commodity_id.clone())) | ||
| .or_insert_with(|| { | ||
| model.agents[agent_id].commodity_portions[&(commodity_id.clone(), year)] | ||
| }); |
There was a problem hiding this comment.
Seems a bit nitpicky to me
| // Retrieve installable capacity limits for flexible capacity assets. | ||
| let key = (commodity_id.clone(), year); | ||
| let mut agent_share_cache = HashMap::new(); | ||
| let capacity_limits = flexible_capacity_assets | ||
| .iter() | ||
| .filter_map(|asset| { | ||
| let agent_id = asset.agent_id().unwrap(); | ||
| let commodity_id = asset.primary_output_commodity().unwrap(); | ||
| let agent_share = *agent_share_cache | ||
| .entry(agent_id.clone()) | ||
| .or_insert_with(|| model.agents[agent_id].commodity_portions[&key]); | ||
| .entry((agent_id, commodity_id.clone())) | ||
| .or_insert_with(|| { | ||
| model.agents[agent_id].commodity_portions[&(commodity_id.clone(), year)] | ||
| }); |
There was a problem hiding this comment.
I tried adding a patch version of circularity with a larger circuit covering more commodities, but I couldn't get it to work and need more work to figure out why
Description
This function currently calculates asset capacity limits based on the commodity portion for the commodity we're currently looking at. However, since this function iterates over multiple commodities and progressively expands the asset pool, this may be incorrect for assets invested in in previous iterations. We need to look at the commodity portion relevant for the agent and primary commodity of each asset.
agent_share_cachenow keyed by (agent, commodity), since agents can have different shares for different commodities.Important if commodities in a cycle are owned by different agents (currently it would panic)
Fixes # (issue)
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks