-
Notifications
You must be signed in to change notification settings - Fork 3
Fix commodity price calculation #445
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
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
e4f77a6
Add framework for retrieving capacity duals
tsmbland 05a0b85
Remove iter_commodity_prices
tsmbland bbe989a
Working (but messy) calculation
tsmbland 2161088
Rename some variables
tsmbland 9bd70de
Clarify imline comments
tsmbland c113ae4
Change order of dual insertion
tsmbland fe0b628
Reorder code for readability
tsmbland 38f2a75
Combine loops for price calculation
tsmbland cb1d448
Disable link check
tsmbland 7b8a3e4
Merge branch 'main' into commodity_prices
tsmbland 3bc69ec
Merge branch 'main' into commodity_prices
tsmbland File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
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.
@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?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.
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 toRc<Asset>s for the constraint "keys". We'd also have to changeAssetPoolto storeRc<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 useWeaks 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.)