-
Notifications
You must be signed in to change notification settings - Fork 53
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
Supply Curve Aggregation by Zone within GIDs #503
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
==========================================
+ Coverage 87.61% 87.75% +0.14%
==========================================
Files 121 121
Lines 18390 18536 +146
==========================================
+ Hits 16112 16267 +155
+ Misses 2278 2269 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Leaving comments here now so that I don't forget them over the weekend
…ically/likely boolean
…one_mask() and replacing with logic in __init__
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.
This is awesome, thank you! Couple of quick questions and then I think we can get this merged!
assert zone_mask.shape[0] <= resolution, msg | ||
assert zone_mask.shape[1] <= resolution, msg | ||
assert zone_mask.size == len(self._gids), msg | ||
self._zone_mask = zone_mask.copy() |
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.
This copy seems to be intentionally placed, but I can't seem to figure out why. Do you ever modify the values within? Did you run into some sort of issues without the copy? or is this just trying to be future-proof?
# each entire cell is one zone | ||
for gid, gid_slice in slice_lookup.items(): | ||
data[gid_slice] = gid + 10 |
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.
Would it be worth having a separate test with multiple zones per SC point? Just to make sure we get repeated sc_point_gid
outputs with different zone ID's? Or is that too cumbersome to set up?
First cut at implementing supply curve aggregation for zones within GIDs. This functionality is enable by passing values to the new
zones_dset
argument.I added new tests to
tests/test_supply_curve_sc_aggregation.py
, and the existing tests are all passing as well. I was unable to directly test that this functionality works withcap_cost_scale
because the test gen/econ datasets do not have the correctly named economic datasets, but I did test that each zone + sc site is output to a separate sc_gid with the correct areas and capacities. Since the capital cost scale functionality is already tested separately, and runs downstream of the area and capacity calculations for each site inGenerationSupplyCurvePoint.summarize()
, I don't this its okay not to test it.This PR also includes some incidental changes to
.pylintrc
and.pre-commit-config.yaml
to get them working again (they were pretty outdated).