-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add ability to scale gencost from change table #322
Conversation
3aeba7c
to
2d47fc1
Compare
I generalized the way we add plant_id- & zone_id-specified info to the change table, so that we can re-use the same code for scaling capacities as we do for scaling costs (and we will use the same for scaling Pmins). I think this is ready to review. |
The |
3fb8c2f
to
f216bfb
Compare
Let me know what you want to do for the tests. the patch you propose can be done here or in a following PR. I went through the code and it looks good. |
""" | ||
self._add_plant_entries(resource, resource, zone_name, plant_id) | ||
|
||
def scale_plant_cost(self, resource, zone_name=None, plant_id=None): |
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.
Should we prevent users from trying to scale a zero cost resource here such as wind, solar, hydro, etc, or simply put down some warnings?
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
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.
Will these resources be zero-cost forever? With production tax credits, they may actually be bidding negative prices.
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.
Do we have cost curves for renewable generators with zero coefficients in gencost
?
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.
Yes, they have 0
for c0
, c1
, c2
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.
If they are getting a $10/MWh subsidy, then I think their 'true' cost is -10 $/MWh.
But they may not want to expose this subsidy in the market.
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.
Aha, private subsidies, now we are getting strategic!
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.
Coming back to this: I think there are other places in the code where we've baked in the idea that renewables are always zero cost. E.g. in transform_grid.py: if gen_type in self.thermal_gen_types:
, we are implicitly assuming that non-'thermal' generators have zero c0
and c2
at the very least, and so there is no need to adjust those cost curve parameters. I like the idea of either making this assumption explicit (e.g. adding a set of 'zero_cost_resources' somewhere) or to stop assuming it altogether.
But, in my own self-interest, I would like to say that this is outside of the scope of this PR.
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.
I see. Does it need to be handled here?
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.
If we decide to set renewables to zero cost, a good place to define the zero_cost_resources
would be in powersimdata/network/usa_tamu/constants/plants.py
The comments here don't seem too tough to address, I would advocate for merging this and then fixing the tests in a follow-up. |
3dff028
to
58d0f48
Compare
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.
Looks good. I have created an issue (#332) to address the equivocalness of zero cost for renewable generators
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.
Looks good to me ;)
58d0f48
to
9cf3d94
Compare
Purpose
Add ability to scale gencost from change table. Closes #142.
What the code is doing
change_table.py
:_add_plant_entries()
to add generator transformations to the change table. This is largely a copy/paste of the existingscale_plant_capacity
, with one new added parameter that determines the top-level key that the plant_id and zone_name sub-dicts will live in. I also changed the behavior for bad info to be to re-raise the Exception after clearing the change table entry, rather than just printing the error & returningNone
; I think this will prevent users from overlooking these errors.scale_plant_capacity
to use this new method.scale_plant_cost()
that also uses this method, with the difference that the keys that are added are e.g. "coal_cost" rather than "coal".transform_grid.py
:_scale_gencost()
to_scale_gencost_by_capacity()
to emphasize that this is being done as a subroutine of_scale_gen()
, since this is done to preserve the shape of the cost curve when we simulate adding new capacity by scaling up existing generators, and_scale_gencost()
method which scales all cost coefficients equally.test_transform_grid.py
: adding tests that these methods work as expected.Time estimate
30 minutes. Not much is actually changed, and most new code is copy/pasted from existing code.
There's one somewhat-related issue I found in the rest of the
test_transform_grid.py
code: if the TransformGrid init andget_grid()
fails in one test, then it may cause unrelated tests to fail, since the ChangeTable will not get cleared. Maybe other ways too. To demonstrate, try running the tests with araise Exception
as the first line ofTransformGrid._scale_gencost()
: it will break unrelated tests.I think we can fix this by moving all code that touches
ct
into thetry:
/finally:
blocks, or alternatively by just using a deepcopy within each individual test. Probably other ways too. We could address this here or in a separate PR.