New price strategies + allow to vary by commodity

[tsmbland]

Description

This PR has a couple of changes addressing the way that commodity prices are calculated.

The first change is to allow the pricing strategy to vary by commodity. To do this, I've taken the pricing_strategy parameter away from model.toml, and added a new column to the commodities.csv file. The options are similar to before, plus a couple of new strategies I'll mention below, and with two more new options:

• "unpriced": since we have to give an option for all commodities, but there are commodities for which we won't/shouldn't calculate a price, I needed an option to represent this. In particular this will be all OTH type commodities (e.g. CO2, wind) which don't have supply/demand balance so it's meaningless to calculate a price
• "default": I figured many users probably won't care about the subtleties of the pricing strategy, so this is an option for these people (will default to "shadow" for SED/SVD commodities, and "unpriced" for OTH commodities)

There is validation in place to ensure that an appropriate strategy is chosen according to the commodity type. I might consider allowing "unpriced" for SVD commodities in the future, because I think for example if it's something like "vehicle miles" then it doesn't really make sense to give it a price.

When it comes to calculating prices, it turned out easiest/most efficient to group the commodities together according to their pricing strategy, calculate prices for each group in batch, then combine.

Secondly, as already mentioned, there are some new pricing strategies:

• "full": following the suggestion by Adam here (and see further down comments), a price strategy that includes capital costs.
• "marginal": the same as above, but without the fixed cost component. Not specifically requested, but I thought it would be useful to include anyway.

I've updated the example models to use the "full" strategy where it seems appropriate (most commodities except for electricity)

Fixes #1009
Fixes #1013

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

❌ Patch coverage is 63.91129% with 179 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.71%. Comparing base (6627450) to head (dc7a617).
⚠️ Report is 55 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/prices.rs	52.32%	125 Missing and 8 partials ⚠️
src/asset.rs	79.41%	21 Missing ⚠️
src/input/commodity.rs	83.72%	10 Missing and 4 partials ⚠️
src/simulation/optimisation.rs	23.07%	10 Missing ⚠️
src/simulation.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1021      +/-   ##
==========================================
- Coverage   81.44%   80.71%   -0.73%     
==========================================
  Files          52       52              
  Lines        6494     6924     +430     
  Branches     6494     6924     +430     
==========================================
+ Hits         5289     5589     +300     
- Misses        948     1063     +115     
- Partials      257      272      +15     

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

[alexdewar]

This seems sensible in general but I've got some questions/suggestions. I'd probably have split this into two PRs, so that we could review the thing about making pricing strategies for commodities configurable first. It would have been easier to say "yes" to that, without worrying about the implications of the new pricing strategies etc... but I'm not sure what order you did things in. Anyway, it doesn't matter much.

I've had a look at the issue, but would you mind just saying again briefly what it is that the "full" strategy gives us that the simple "shadow" one doesn't? When is it appropriate to use each? I know it came from Adam (and I trust him!), but I'd just like to see if I can get it a bit clearer in my own head. (I feel like this is one of those things where I won't totally wrap my head around it until I'm actually working on the code, but I'd like to try!)

As a result, my review of the new pricing strategy code is a bit superficial and I've just trusted that most of the logic is correct. I feel like it would help if some of the new functions/methods had tests, even if they're AI-generated. (I think this is especially true for the simpler ones, actually, because then it's easy to verify that they're doing what they're supposed to from looking at the tests.)

I've got a few thoughts about the way you've explicitly set the pricing_strategy for the commodities for of the examples. In general, are we thinking that manually setting the pricing strategy is something that users should be doing, even if they're beginners? If it's more of an advanced feature, then I think it would be better to just have the pricing_strategy column be empty for most of the examples (meaning "let MUSE2 pick a sensible default for you"), unless where it's strictly necessary for the model to run. We could have a separate "advanced" example that showcases this feature, if we wanted. My worry is that users will just blindly copy and paste these examples and not really pay attention to the whole "pricing strategy" thing and so end up with a random mix of strategies that doesn't make sense and end up with nonsensical results. Maybe this is something that will be much more obvious to someone who actually knows something about economics, but I currently wouldn't be confident choosing an appropriate strategy myself at this point, despite having read your discussion with Adam. For most things in the example CSV files, I think it makes sense to be explicit and fill in all the fields, even where they could be left blank, but, if this really is more of an advanced feature that many users might not understand, I think it's asking for trouble.

It seems like there may be a conceptual issue with using the "full" pricing strategy for commodities with multiple outputs. There are a few ways we could handle this:

1. Assume it's fine for now and just allow it (what we're doing currently)
2. Forbid users from setting pricing_strategy to full for commodities with multiple outputs (possibly unless the "broken options" flag is set)
3. Just warn users if they do this, but not forbid it (we could open a GitHub issue/discussion for it and link to this in the log message like we've done elsewhere)

Just wondering if we want to consider doing 2 or 3 instead?

Finally: I'm wondering if it would be useful for users to be able to configure the default default pricing strategy for different commodity types. Could do it with a "table" type in model.toml so users could write:

default_pricing_strategy = { sed = "shadow" }

(Or equivalently they could write it as a separate section.)

[tsmbland]

Thanks @alexdewar!

I've started with the easy suggestions and will move on to the harder ones next

[tsmbland]

This seems sensible in general but I've got some questions/suggestions. I'd probably have split this into two PRs, so that we could review the thing about making pricing strategies for commodities configurable first. It would have been easier to say "yes" to that, without worrying about the implications of the new pricing strategies etc... but I'm not sure what order you did things in. Anyway, it doesn't matter much.

I did this first (8e9060d), however it was pretty simple and I was fairly sure it would end up changing a lot after adding the new pricing methods (which, indeed, it did)

I've had a look at the issue, but would you mind just saying again briefly what it is that the "full" strategy gives us that the simple "shadow" one doesn't? When is it appropriate to use each? I know it came from Adam (and I trust him!), but I'd just like to see if I can get it a bit clearer in my own head. (I feel like this is one of those things where I won't totally wrap my head around it until I'm actually working on the code, but I'd like to try!)

The "full" strategy includes a contribution from capital costs in the price, which the "shadow" method doesn't. That's mostly the extent of my understanding. The rest of it equates to the difference between the "marginal" and "shadow" methods, which should in theory be similar (i.e. looking at the marginal cost of the most expensive producer), but they're calculated in fundamentally different ways and do sometimes give different prices.

As a result, my review of the new pricing strategy code is a bit superficial and I've just trusted that most of the logic is correct. I feel like it would help if some of the new functions/methods had tests, even if they're AI-generated. (I think this is especially true for the simpler ones, actually, because then it's easy to verify that they're doing what they're supposed to from looking at the tests.)

Maybe. I generally avoid writing tests that would require horribly complex fixtures, even with AI, because I think these can be more of a burden than a benefit if we ever want to change the code in the future

I've got a few thoughts about the way you've explicitly set the pricing_strategy for the commodities for of the examples. In general, are we thinking that manually setting the pricing strategy is something that users should be doing, even if they're beginners? If it's more of an advanced feature, then I think it would be better to just have the pricing_strategy column be empty for most of the examples (meaning "let MUSE2 pick a sensible default for you"), unless where it's strictly necessary for the model to run. We could have a separate "advanced" example that showcases this feature, if we wanted. My worry is that users will just blindly copy and paste these examples and not really pay attention to the whole "pricing strategy" thing and so end up with a random mix of strategies that doesn't make sense and end up with nonsensical results. Maybe this is something that will be much more obvious to someone who actually knows something about economics, but I currently wouldn't be confident choosing an appropriate strategy myself at this point, despite having read your discussion with Adam. For most things in the example CSV files, I think it makes sense to be explicit and fill in all the fields, even where they could be left blank, but, if this really is more of an advanced feature that many users might not understand, I think it's asking for trouble.

Yeah I think that's fair, it's more of an advanced feature. But I actually think using "shadow" for everything isn't appropriate, which is why I changed all the examples (also because I was interested to see the outcome). Perhaps we should change the default to "full":

• Based on my limited knowledge of economics, it seems entirely reasonable that capital costs would be factored into prices in most cases
• Adam advised to use it for all commodities except electricity
• Perhaps a bit explain how it's calculated by breaking it down into its components (unlike the "shadow" method which requires understanding the concept of duals)
• Anecdotally it seems to behave better for processes with multiple outputs, where the shadow method can often give prices of zero
• It's much closer to the method used in MUSE1, so will be easer to grasp for any users coming from MUSE1

Downsides:

• we haven't tested it as much
• more work to calculate, rather than just directly picking off duals
• it actually uses the shadow prices within the calculation, so users may still need to understand what these are
• Adam pointed out an issue that could lead to very high prices via this method (Add a price option that accounts for capital costs #1009 (comment)), which isn't fully solved, although the partial decommissioning WIP will likely help

Anyway, I guess for now we should keep the default as "shadow" and change everything back so at least the behaviour is the same as before, then we can think about this later (and loop Adam into that discussion).

It seems like there may be a conceptual issue with using the "full" pricing strategy for commodities with multiple outputs. There are a few ways we could handle this:

1. Assume it's fine for now and just allow it (what we're doing currently)
2. Forbid users from setting pricing_strategy to full for commodities with multiple outputs (possibly unless the "broken options" flag is set)
3. Just warn users if they do this, but not forbid it (we could open a GitHub issue/discussion for it and link to this in the log message like we've done elsewhere)

Just wondering if we want to consider doing 2 or 3 instead?

Actually, so long as the output commodities all have the same units (e.g. all PJ or all kg, but not a mix), then I don't think there's a problem here, and it actually seems to behave better than the shadow method which often gives lots of zero prices. This is why I suggested adding a units column to the commodities file, which Adam seems on board with (#1009 (comment))

Finally: I'm wondering if it would be useful for users to be able to configure the default default pricing strategy for different commodity types. Could do it with a "table" type in model.toml so users could write:

default_pricing_strategy = { sed = "shadow" }

(Or equivalently they could write it as a separate section.)

Hmm yeah that could be useful! For another PR though

[alexdewar]

LGTM. Good work! Found one small thing.

Seems sensible to iron out the kinks with the full strategy before we roll it out.

[alexdewar]

Suggested change

[alexdewar]

Suggested change

	enum: [shadow, marginal, full, scarcity, unpriced, default]
	enum: [shadow, marginal, full, scarcity, unpriced]

[tsmbland]

Great, thanks. I've put the new strategies behind please_give_me_broken_results for now, until we're more confident that they work properly