Skip to content

Fix more broadcasting errors#534

Merged
tsmbland merged 7 commits intov1.2.2from
broadcast_errors2
Oct 25, 2024
Merged

Fix more broadcasting errors#534
tsmbland merged 7 commits intov1.2.2from
broadcast_errors2

Conversation

@tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented Oct 24, 2024

Description

This PR fixes a few more bugs caused by automatic broadcasting.

I identified these using the approach suggested by @dalonsoa here, which works beautifully. It essentially turns off automatic broadcasting (I've limited this to the timeslice dimension), and throws an error message when broadcasting would be required to perform the intended operation. I've implemented this in a separate PR/branch (#530). I then ran the code in that branch and went through every place where it complained about automatic broadcasting, and was able to spot a few more places where convert_timeslice should be applied to split quantities over the timeslice dimension (rather than automatic broadcasting as was being done previously).

I then went back and applied all of these changes here. Long run we'll want to merge #530 to properly turn off automatic broadcasting (rather than just fixing errors like I'm doing here). However, there are lots of places where it ends up complaining about benign things, and it will still take some work to make it completely happy. Right now I just want to fix these bugs as quickly as possible. (#530 is also way ahead of this one and has a completely different timeslices module which isn't ready yet.)

Summary of the changes:

  • Fix an error in the ALCOE function where fuel costs and env costs for the whole year were applied to each timeslice. This ends up effecting commodity prices
  • Fix a similar error in the emission_cost objective. This effects investment results in the agent tutorial which uses this objective
  • Fix a similar error in gross_margin with the calculation of consumption and production costs. I believe this function isn't used in any of the examples models, but it still might as well be correct

Fixes # (issue)

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass: $ python -m pytest
  • The documentation builds and looks OK: $ python -m sphinx -b html docs docs/build

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@tsmbland tsmbland mentioned this pull request Oct 24, 2024
8 tasks
@tsmbland tsmbland marked this pull request as ready for review October 24, 2024 16:00
@tsmbland tsmbland requested a review from dalonsoa October 24, 2024 16:01
Base automatically changed from fix_supply_issue to v1.2.2 October 25, 2024 11:26
Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things look good - specially, if they work!!

I would take this opportunity to write some notes here and there clarifying what's going on. Sometimes, MUSE code is a bit like black magic and difficult to understand what's happening. So, for those functions you are modifying, anyway, add a bit of extra context.

Comment on lines 392 to 402
fuel_costs = (
convert_timeslice(techs.fixed_inputs, prices.timeslice, QuantityType.EXTENSIVE)
* prices
).sum("commodity")

fuel_costs += (
convert_timeslice(
techs.flexible_inputs, prices.timeslice, QuantityType.EXTENSIVE
)
* prices
).sum("commodity")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add some notes. I missed that one was fixed and the other was flexible. Something along the lines of...

Suggested change
fuel_costs = (
convert_timeslice(techs.fixed_inputs, prices.timeslice, QuantityType.EXTENSIVE)
* prices
).sum("commodity")
fuel_costs += (
convert_timeslice(
techs.flexible_inputs, prices.timeslice, QuantityType.EXTENSIVE
)
* prices
).sum("commodity")
# First we calculate the FIXED costs
fuel_costs = (
convert_timeslice(techs.fixed_inputs, prices.timeslice, QuantityType.EXTENSIVE)
* prices
).sum("commodity")
# And the the FLEXIBLE costs
fuel_costs += (
convert_timeslice(
techs.flexible_inputs, prices.timeslice, QuantityType.EXTENSIVE
)
* prices
).sum("commodity")

Having said that, shouldn't the flexible costs depend on the production?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fuel costs (and any other flexible costs) will depend on production, but it looks like this function is essentially calculating the costs of installing one unit of the technology operating at full capacity. This is why all the exponents are also missing. Whether or not that's the intention I'm not sure...

I also think the treatment of flexible inputs is wrong. Flexible inputs are alternative fuels (i.e. it can use one or the other), so you shouldn't be adding them all together.

@dalonsoa
Copy link
Collaborator

Some tests are failing, so they would need fixing, of course.

@tsmbland tsmbland merged commit be29358 into v1.2.2 Oct 25, 2024
@tsmbland tsmbland deleted the broadcast_errors2 branch October 25, 2024 15:16
@tsmbland tsmbland mentioned this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants

Comments