Give candidate assets an agent ID

[alexdewar]

Description

As part of the series of fixes for reduced costs (#876), we will soon be transitioning to using the same method for calculating reduced costs as we currently do for existing costs (#879). This means that candidate assets will need to be associated with a particular agent, as the reduced cost calculation will vary depending on the agent's objective (see my other PR: #882). Currently, candidate assets are agent-less unless and until they are selected by an agent, at which point their state is changed to AssetState::Selected and they are assigned an agent ID. All other asset types already have an associated agent ID; this PR just changes things so candidates have one too, by making agent_id a field of Asset rather than a value embedded in different AssetState variants.

This means we have to use a different method for generating candidate assets for a given year: now we iterate over agents and look at their search spaces, so we have a corresponding agent for each, rather than just looking at the set of processes which can be commissioned in a given year instead.

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

[Copilot]

Pull Request Overview

This PR refactors the asset system to give candidate assets an agent ID from creation, moving the agent_id field from being embedded within different AssetState variants to being a direct field of the Asset struct. This change prepares for upcoming reduced cost calculation improvements by ensuring all assets have an associated agent.

Key changes:

• Moved agent_id from AssetState variants to Asset struct field
• Updated candidate asset generation to iterate over agents and their search spaces
• Simplified asset state transitions by removing agent ID assignment during selection

Reviewed Changes

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

File	Description
src/asset.rs	Refactored Asset struct to include agent_id field and simplified AssetState enum variants
src/simulation.rs	Updated candidate asset generation logic to iterate over agents and their search spaces
src/simulation/investment.rs	Modified asset selection logic to work with new agent ID structure
src/output.rs	Updated asset output to use new direct agent ID access

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

[nitpick] The comparison asset.state() == &AssetState::Candidate creates an unnecessary reference. Consider using matches!(asset.state(), AssetState::Candidate) for better readability and consistency with the pattern used elsewhere in the codebase.

Suggested change

	if asset.state() == &AssetState::Candidate {
	if matches!(asset.state(), AssetState::Candidate) {

[alexdewar]

I don't think this is really cleaner...

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.23%. Comparing base (e8f72c7) to head (70f0916).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #883      +/-   ##
==========================================
+ Coverage   85.18%   85.23%   +0.04%     
==========================================
  Files          50       50              
  Lines        5300     5317      +17     
  Branches     5300     5317      +17     
==========================================
+ Hits         4515     4532      +17     
  Misses        565      565              
  Partials      220      220              

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

[dalonsoa]

I think all makes sense. Is this approach increasing the pool size of candidate assets or it is the same but it is created in such a way that the agent ID can be added to them? I'm not entirely sure by reading through the loops involved.

[dalonsoa]

Nothing to do with this PR, but I'm super confused with the use of self. Sometimes, I see self.state, like here. But below we use self.0.state. Where is this 0 coming from?

[alexdewar]

You can use self.0 to get at the inner type if your object is what's called a newtype in Rust, which is basically just a struct with only one, unnamed field. So we do it for CommodityPrices, for example, which is a wrapper around a map type with some extra methods etc.

[alexdewar]

I think all makes sense. Is this approach increasing the pool size of candidate assets or it is the same but it is created in such a way that the agent ID can be added to them? I'm not entirely sure by reading through the loops involved.

It could do, though I think it practice it won't make much difference. Now we look at every process that can be commissioned in a given year by each agent, which means if there are two agents that could commission one particular type of process, then you will now have two candidate assets (one per agent) rather than one. But I don't know how common that situation is: in our simple examples every type of process can only be commissioned by one agent anyway, so it won't matter. @tsmbland probably has a better sense of how often this happens with real models.

[tsmbland]

I could swear there was a good reason that we didn't give candidate assets an agent ID before... but I can't recall what that was? @alexdewar Do you remember? Was it to do with hashing?

[alexdewar]

Kind of... I think the thing is we were generating the list of candidate assets from processes directly, so we didn't really have an agent ID to give them. As mentioned above, it is possible you could have a candidate that could be selected by more than one agent, so it seemed fine to have candidate assets be agent-less. But the agent will matter when we calculate RCs from them (or whatever we're calling them) because the objective type changes it.

Except. If we take your suggestion about refactoring the appraisal to calculate coeffs upfront, then candidates can still be agent-less, as the calculation will be happening later on when we know whether we're using NPV or LCOX anyway, in which case we wouldn't need this change.

Hmm. What do we think?

[tsmbland]

Kind of... I think the thing is we were generating the list of candidate assets from processes directly, so we didn't really have an agent ID to give them. As mentioned above, it is possible you could have a candidate that could be selected by more than one agent, so it seemed fine to have candidate assets be agent-less. But the agent will matter when we calculate RCs from them (or whatever we're calling them) because the objective type changes it.

Except. If we take your suggestion about refactoring the appraisal to calculate coeffs upfront, then candidates can still be agent-less, as the calculation will be happening later on when we know whether we're using NPV or LCOX anyway, in which case we wouldn't need this change.

Hmm. What do we think?

Yep, I think we should do this instead. @dalonsoa point about increasing the size of the asset pool is a big one - there could be very many agents per candidate and this would needlessly expand the size of the dispatch optimisation

[alexdewar]

Yep, I think we should do this instead. @dalonsoa point about increasing the size of the asset pool is a big one - there could be very many agents per candidate and this would needlessly expand the size of the dispatch optimisation

Sounds like a plan. I'll revert 3edbd42, which gives candidates asset IDs, then merge this anyway so we get the other little tidy-ups from this branch.