Fix commodity price calculation

[tsmbland]

Description

Implements the new commodity price calculation as suggested by @ahawkes in #433

Main changes to the code:

• created iter_capacity_duals to retrieve the dual values for the capacity constraints
• added a check in add_asset_capacity_constraints to make sure these constraints are added immediately after the commodity balance constraints
• implemented the new commodity price calculation in add_from_solution

I still have a couple of questions:

• should we add capacity constraint duals only for PACs? (I have assumed yes, as I wouldn't think this would apply to side-products such as CO2)
• do we need to do anything different for processes with multiple output commodities? It seems to me that this would have some impact on commodity prices (see this discussion for MUSE1), although maybe this is already factored into the duals? I have to say I don't have a strong enough grasp on linear algebra at this stage to fully understand this.

I think it's probably ok to leave these as open questions for now, and revisit when we come to look at more complex models

I haven't added any tests, but we currently have no tests anyway for the constraints

Fixes #433

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

[codecov]

Codecov Report

Attention: Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.53%. Comparing base (bb5cb56) to head (3bc69ec).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/optimisation.rs	97.91%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #445      +/-   ##
==========================================
+ Coverage   95.50%   95.53%   +0.03%     
==========================================
  Files          31       31              
  Lines        4515     4571      +56     
  Branches     4515     4571      +56     
==========================================
+ Hits         4312     4367      +55     
- Misses        100      101       +1     
  Partials      103      103              

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

[tsmbland]

@alexdewar Could you have a look at this and we can talk about it on Monday? It works and gives the expected results, but it's definitely wasteful and I could use some advice from someone with more Rust experience

[alexdewar]

Sure. I only just noticed this, but I'll take a look first thing on Mon.

[alexdewar]

I've made a couple of suggestions. The main one is that I think you can combine the first and last loops in CommodityPrices::add_from_solution. In general the approach looks good though and it's nice clean code.

[alexdewar]

A HashMap is fine here, because we don't care about ordering:

Suggested change

	let mut highest_duals: IndexMap<CommodityPriceKey, f64> = IndexMap::new();
	let mut highest_duals = HashMap::new();

(I also don't think you need the type hints here, though the compiler might not agree! 😛)

See: https://crates.io/crates/indexmap

[tsmbland]

@alexdewar Thanks for the comments! The only other thing I'm unsure about is this loop, as it's repeating a lot of asset-level computations for every timeslice (getting the asset, getting the pacs, checking if the flows are positive). I think better would be to have nested loops, but that would require re-designing iter_capacity_duals, and maybe it's not even worth it for the extra work / trade-off with readability. What do you think?

[alexdewar]

Good point... I don't think there's a way around that without making the data structures more complicated though, so I'd leave it as is for now. We can always optimise this stuff further down the line if benchmarking shows it's a bottleneck.

One thing we could do to avoid having to look up the assets multiple times (though I don't think we should do it just yet) is to switch from saving AssetIDs to Rc<Asset>s for the constraint "keys". We'd also have to change AssetPool to store Rc<Asset>s etc. If we did this, we'd have to be careful that code wasn't making use of assets after they'd been decommissioned though (wouldn't be a problem for this bit of code). Could use Weaks for that, but that would make things a bit more fiddly.

We could also improve performance for finding PACs. The way we store commodity flows is not particularly optimal in general, so I've opened an issue for it (#449). (Who knows how much impact fixing that would actually have on overall program performance, but it's not an especially big change so we should probably do it at some point.)

[alexdewar]

The code looks good to me... I have no idea about your questions though 🙃. I think this will be fine for now and we can figure out some of the other details further down the line, as you say.

You could write a test for this, though I appreciate it might be fiddly. (If you changed the function signature to take the duals as separate arguments that might make it slightly easier.) In general there aren't any tests for the dispatch optimisation stuff yet though and I think we'll want to consider how we approach it before we add them (#448). Probably something to add to the to-do list for the next engagement.

[tsmbland]

@alexdewar Thanks for the review! I'm going to merge and will think about tests... another time 😂