Skip to content
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

Added computation of cumulative minimum capacity retirements in multistage #542

Merged

Conversation

filippopecci
Copy link
Collaborator

Fixed computation of cumulative minimum retirements in multistage code. Previously, minimum capacity retirement parameters were not added up and this resulted in the minimum retirement constraints enforcing wrong lower bounds ( #514 )

@@ -22,6 +22,56 @@ function get_retirement_stage(cur_stage::Int, lifetime::Int, stage_lens::Array{I
return Int(ret_stage)
end

function compute_min_cumulative_retirements!(t::Int,inputs_d::Dict)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the code easier to read, consider adding a simple one-line function like

dfgen(stage) = inputs_d[stage]["dfGen"]

and also

dfvrestore(stage) = inputs_d[stage]["dfVRE_STOR"]

This also has the advantage that it makes the interface smaller. If you ever want to change "dfVRE_STOR" to another key it's easier since it's only in one place.

Copy link
Collaborator

@cfe316 cfe316 Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also do something like

function xxx(a,b,c):
    if !isempty(a)
        inputs[...][!, Symbol(b)] = sum( .... Symbol(c) for p in 1:t)
    end
end
     
table = [("VS_DC", "Min_Cum_Retired_Cap_Inverter_MW", "Min_Retired_Cap_Inverter_MW"),
              (..., ..., ...),
              ]
for row in table:
    a,b,c = row
    xxx(a,b,c)
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more suggestions to improve readability in addition to @cfe316 comments:

Suggested change
function compute_min_cumulative_retirements!(t::Int,inputs_d::Dict)
function compute_min_cumulative_retirements!(inputs_d::Dict, t::Int)

@@ -22,6 +22,56 @@ function get_retirement_stage(cur_stage::Int, lifetime::Int, stage_lens::Array{I
return Int(ret_stage)
end

function compute_min_cumulative_retirements!(t::Int,inputs_d::Dict)

inputs_d[t]["dfGen"][!,Symbol("Min_Cum_Retired_Cap_MW")] = sum(inputs_d[p]["dfGen"][!,Symbol("Min_Retired_Cap_MW")] for p=1:t);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To improve performance, If t can be very large, we could think about a way to avoid redoing the sum over all the keys 1:t of the dictionary, since the sum over 1:t-1 is stored at inputs_d[t-1]["dfGen"][!,:Min_Cum_Retired_Cap_MW]. Am I correct?

end

if !isempty(inputs_d[1]["VRE_STOR"])
if !isempty(inputs["VS_DC"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo?

Suggested change
if !isempty(inputs["VS_DC"])
if !isempty(inputs_d[1]["VS_DC"])

@filippopecci filippopecci force-pushed the endogenous_retirements_bugfix branch from 60e16b3 to 835cef0 Compare September 26, 2023 20:51
@filippopecci
Copy link
Collaborator Author

Thank you @cfe316 and @lbonaldo for your suggestions. I have implemented all of them, have a look and let me know.

@filippopecci filippopecci force-pushed the endogenous_retirements_bugfix branch from 835cef0 to fa1fc0f Compare September 27, 2023 15:52
CHANGELOG.md Outdated
@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

- Fix computation of cumulative minimum capacity retirements in multistage GenX (#514)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be under a ### Fixed section of the changelog. If we don't have one, add it below ### Added. See https://keepachangelog.com/en/1.1.0/ .

Comment on lines 42 to 56
if isempty(inputs_d[1]["VRE_STOR"])
mytab =[("G","dfGen",:Min_Retired_Cap_MW),
("STOR_ALL","dfGen",:Min_Retired_Energy_Cap_MW),
("STOR_ASYMMETRIC","dfGen",:Min_Retired_Charge_Cap_MW)
]
else
mytab =[("G","dfGen",:Min_Retired_Cap_MW),
("STOR_ALL","dfGen",:Min_Retired_Energy_Cap_MW),
("STOR_ASYMMETRIC","dfGen",:Min_Retired_Charge_Cap_MW),
("VS_DC","dfVRE_STOR",:Min_Retired_Cap_Inverter_MW),
("VS_SOLAR","dfVRE_STOR",:Min_Retired_Cap_Solar_MW),
("VS_WIND","dfVRE_STOR",:Min_Retired_Cap_Wind_MW),
("VS_STOR","dfGen",:Min_Retired_Energy_Cap_MW),
("VS_ASYM_DC_DISCHARGE","dfVRE_STOR",:Min_Retired_Cap_Discharge_DC_MW),
("VS_ASYM_DC_CHARGE","dfVRE_STOR",:Min_Retired_Cap_Charge_DC_MW),
("VS_ASYM_AC_DISCHARGE","dfVRE_STOR",:Min_Retired_Cap_Discharge_AC_MW),
("VS_ASYM_AC_CHARGE","dfVRE_STOR",:Min_Retired_Cap_Charge_AC_MW)
]
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the first mytab a subset of the second one? Could this be converted to

mytab = ...

if VRESTORE
   add new entries to my_tab
end

Then they'd only need to be listed once.

Comment on lines +29 to +34
if !isempty(inputs_d[1][Resource_Set])
if t==1
inputs_d[t][dfGen_Name][!,CumRetCap] = inputs_d[t][dfGen_Name][!,RetCap];
else
inputs_d[t][dfGen_Name][!,CumRetCap] = inputs_d[t-1][dfGen_Name][!,CumRetCap] + inputs_d[t][dfGen_Name][!,RetCap];
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the second line here is the sum of the first line plus the term inputs_d[t-1][dfGen_Name][!,CumRetCap]

Could this be reformulated to

A = B
if t==1
   A += C
end

@filippopecci filippopecci force-pushed the endogenous_retirements_bugfix branch from fa1fc0f to 6edc3b0 Compare October 24, 2023 19:43
…e. Previously, minimum capacity retirement parameters were not added up and this resulted in the minimum retirement constraints enforcing wrong lower bounds

Please enter the commit message for your changes. Lines starting
@filippopecci filippopecci force-pushed the endogenous_retirements_bugfix branch from 6edc3b0 to 0ab56d8 Compare October 24, 2023 21:38
Copy link
Collaborator

@cfe316 cfe316 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@filippopecci filippopecci merged commit c7302ad into GenXProject:develop Oct 24, 2023
1 check passed
qluo0320github pushed a commit that referenced this pull request Nov 27, 2023
Fixed computation of cumulative minimum retirements in multistage code. Previously, minimum capacity retirement parameters were not added up and this resulted in the minimum retirement constraints enforcing wrong lower bounds (#542)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants