Skip to content

Fix bug defining subsector commodities when zero existing capacity#703

Merged
tsmbland merged 7 commits intomainfrom
subsector_commodities
May 8, 2025
Merged

Fix bug defining subsector commodities when zero existing capacity#703
tsmbland merged 7 commits intomainfrom
subsector_commodities

Conversation

@tsmbland
Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland commented May 6, 2025

Description

MUSE uses subsectors which are in charge of one or more commodities. Users can explicitly pass a list of commodities in the settings file, or MUSE can infer the commodities based on the ExistingCapacity file.

However, there's a problem with how this is done. MUSE first creates assets from the ExistingCapacity, then aggregates the end-use commodities from these assets. However, if there are technologies in ExistingCapacity with zero initial capacity, no assets will be created for these, and these won't be included in the end-use aggregation process.

E.g. a refineries sector may be in charge of oil and hydrogen. If there's no initial demand for hydrogen, the user will set initial capacity for hydrogen-producing technologies to zero. However, demand for hydrogen may increase in the future, so we still want to include hydrogen in the subsector's self.commodities so that hydrogen demand get's passed to the subsector.

I've fixed this by aggregating the enduse commodities from ExistingCapacity directly.

I've added another step during investment to remove commodities with zero demand. E.g in the above example we wouldn't want it to perform investments to meet hydrogen demand when demand is zero (there will be no investments, but it's just a waste of computation). This is probably a good thing to do anyway, but seemed especially appropriate now.

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 marked this pull request as ready for review May 7, 2025 15:38
@tsmbland tsmbland changed the title Fix error defining subsector commodities when zero existing capacity Fix bug defining subsector commodities when zero existing capacity May 7, 2025
@tsmbland tsmbland requested review from Copilot and dalonsoa May 8, 2025 08:13
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where subsector commodities defined from the ExistingCapacity file were missing when their initial capacity was zero. The changes update the aggregation logic in the subsector and related tests, and adjust agent factory inputs for consistency.

  • Removed the use of agent.assets in aggregate_enduses and now infer commodities directly from technologies.
  • Updated the agent factory to require parsed capacity data.

Reviewed Changes

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

File Description
tests/test_subsector.py Updated tests to call aggregate_enduses with technologies only.
src/muse/sectors/subsector.py Revised commodity aggregation and demand filtering; now aggregates directly from technologies.
src/muse/agents/factories.py Refined the capacity parameter type to be a DataArray (removing support for file paths).

Comment thread src/muse/sectors/subsector.py Outdated
def agents_factory(
params_or_path: str | Path | list,
capacity: xr.DataArray | str | Path,
capacity: xr.DataArray,
Copy link

Copilot AI May 8, 2025

Choose a reason for hiding this comment

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

Since capacity is now strictly expected to be a DataArray, consider updating related documentation and usages to make it clear that file paths are no longer supported.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
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.

LGTM! What an impact for such small change. I guess that none of the examples and tutorials were affected by this, right?

Comment thread src/muse/sectors/subsector.py Outdated
@tsmbland
Copy link
Copy Markdown
Collaborator Author

tsmbland commented May 8, 2025

LGTM! What an impact for such small change. I guess that none of the examples and tutorials were affected by this, right?

Doesn't affect any of the examples, but was picked up by a researcher using a more complicated model. (In fact this is a bug that I accidentally introduced in v1.4, which is why it's only being picked up now 🫢)

@tsmbland tsmbland enabled auto-merge (squash) May 8, 2025 09:32
@tsmbland tsmbland disabled auto-merge May 8, 2025 09:34
@tsmbland tsmbland merged commit c85a114 into main May 8, 2025
13 of 14 checks passed
@tsmbland tsmbland deleted the subsector_commodities branch May 8, 2025 09:35
@github-project-automation github-project-automation Bot moved this to ✅ Done in MUSE May 8, 2025
@tsmbland tsmbland mentioned this pull request May 13, 2025
8 tasks
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.

3 participants