-
Notifications
You must be signed in to change notification settings - Fork 10
Xarray patch: Check array dimensions #647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
dalonsoa
approved these changes
Jan 31, 2025
Collaborator
dalonsoa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is part 1 of two PRs (also #585) to do with the
broadcast_techsfunction and correct formatting of dataTo give some background, technologies datasets (and also other datasets such as prices and demand) can take two formats:
Let's say, for example, you had a capacity dataset for a series of assets:
You want to find the capital costs of installing each asset to this level of capacity. First, you first need to find the appropriate
cap_parvalue for each asset. Then multiply this by the capacity to get overall capital costs.If your
cap_pardataset is in a format like this (i.e. fully-explicit):you first need to select the appropriate value for each asset (i.e. gasboiler/R1 for the first asset and heatpump/R2 for the second asset). You can do this with the
broadcast_techsfunction usingcapacityas a template, which will convert the data to a flattened format:You can then multiply this with your
capacitydataset to get the desired capital costs for each asset (which will be an array with a single "asset" dimension).If you didn't use
broadcast_techsto convert the technology data, you'd end up with separate "asset", "technology" and "region" dimensions after the multiplication, which doesn't make any sense. In fact, any array with an "asset" dimension should never have separate "technology" or "region" dimensions - it this ever crops up it should raise serious alarm bells (see #585 for one such mistake).I'm keen to avoid any mistakes like this going forward, so I thought it was worth investing some time to make sure that these mistakes could never happen. The most robust solution I could think of was to add a check to
patched_broadcast_compat_datato ensure that no array with an "asset" dimension also has a "region" or "technology" dimension (also "installed").This flagged numerous issues in the tests, mostly due to poor design of fixtures, which I have gone through and fixed
Fortunately, only one issue was raised during the regression tests to do with the
supplyfunction in multi-region models. I've fixed this in a separate PR (#585), hence why the regression tests are failing in this PR, but I've kept this separate to make reviewing easier.Also in this PR:
broadcast_techsassetdimension inbroadcast_techs- this will always be "asset" and never anything else, so no need to make it configurablegross_marginand it's test, as this isn't used anywhereI suggest reviewing this and #585 together. Obviously the tests are failing in this PR, but this is fixed in #585. In this PR, I'd suggest focusing on the code in
src, and not worrying too much about the tests as this is all just boring refactoringClose #619