-
Notifications
You must be signed in to change notification settings - Fork 122
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
Refactor reserves #562
Refactor reserves #562
Conversation
5d2c951
to
42fb14c
Compare
This works now. The results for the ISONE_Trizone case (which may be the only standard test case that uses Reserves) are identical when just changing the Overall, using |
f76505b
to
550c55c
Compare
0dfc526
to
d48efd9
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.
Thanks @cfe316. This PR is an excellent refactoring. I checked the logic, and it looks correct to me. I added a few comments to the code that are mostly tiny fixes.
end |
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.
Where is _sum_expression
defined?
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.
That's weird, I don't know.
new_axes = (set, time_range) | ||
expr = JuMP.Containers.DenseAxisArray(aff_exprs.data, new_axes...) | ||
return expr | ||
end |
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.
We should include a test to this functions and add it to expression_manipulation_test.jl
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've added tests now.
@@ -160,136 +160,58 @@ function storage_all_reserves!(EP::Model, inputs::Dict, setup::Dict) | |||
|
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.
CapacityReserveMargin = setup["CapacityReserveMargin"] > 0
eff_down(y) = dfGen[y, :Eff_Down] | ||
|
||
# Maximum storage contribution to reserves is a specified fraction of installed capacity | ||
@constraint(EP, [y in STOR_REG, t in 1:T], vREG[y, t] <= dfGen[y,:Reg_Max] * eTotalCap[y]) |
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.
you could abstract also dfGen[y,:Reg_Max]
@constraint(EP, [y in STOR_REG, t in 1:T], vREG[y, t] <= dfGen[y,:Reg_Max] * eTotalCap[y]) | |
reg_max(y) = dfGen[y,:Reg_Max] | |
@constraint(EP, [y in STOR_REG, t in 1:T], vREG[y, t] <= reg_max(y) * eTotalCap[y]) |
|
||
# Maximum storage contribution to reserves is a specified fraction of installed capacity | ||
@constraint(EP, [y in STOR_REG, t in 1:T], vREG[y, t] <= dfGen[y,:Reg_Max] * eTotalCap[y]) | ||
@constraint(EP, [y in STOR_RSV, t in 1:T], vRSV[y, t] <= dfGen[y,:Rsv_Max] * eTotalCap[y]) |
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.
and dfGen[y,:Rsv_Max]
036933a
to
55a9b88
Compare
Here are the results of the refactor: Costs / 1e9 for the RealSystemExample/ISONE_Trizone case, which includes THERM, HYDRO, VRE, MUST_RUN, and STOR generators.
I then increased
These numbers are very similar; I'd say this was a success. |
55a9b88
to
72cc97a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #562 +/- ##
===========================================
+ Coverage 38.37% 39.22% +0.84%
===========================================
Files 126 126
Lines 6480 6205 -275
===========================================
- Hits 2487 2434 -53
+ Misses 3993 3771 -222 ☔ View full report in Codecov by Sentry. |
Move x/p <= y to x <= p * y This should be a mathematically identical formulation yet gives different numerical results.
Co-authored-by: Luca Bonaldo <[email protected]>
This fixes GenXProject#573, where Reserves + Num_VRE_Bins > 1 would lead to the VRE_NO_POWER_OUT resources being able to contribute an infinite amount to the reserves. This fixes GenXProject#574, where VRE power output constraints were being applied length(VRE) times to all VRE resources, when Reserves = 1.
72cc97a
to
13be2dd
Compare
@cfe316 Thank you for the new commits! Could you please also share the settings of the solver you used to run the example? |
This doesn't quite compile yet for technical reasons related to
add_similar_to_expression!
.I tried refactoring some of the reserve formulations in order to better understand how they should work for fusion, and I think others might appreciate the simplifications.