-
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
Separate new build and retirement eligibility #392
Separate new build and retirement eligibility #392
Conversation
@cfe316 , is this still draft or complete? |
This is not yet complete or ready for review. |
OK !!! |
c6fdd50
to
9accef8
Compare
FYI this will need to be checked for compatibility with the VRESTOR module. |
9accef8
to
79f6f92
Compare
Old format to new formatNew_Build[old] to New_Build[new], Can_Retire |
@@ -30,9 +30,11 @@ function load_generators_data!(setup::Dict, path::AbstractString, inputs_gen::Di | |||
# Set of storage resources with symmetric charge/discharge capacity | |||
inputs_gen["STOR_SYMMETRIC"] = gen_in[gen_in.STOR.==1,:R_ID] | |||
# Set of storage resources with asymmetric (separte) charge/discharge capacity components | |||
inputs_gen["STOR_ASYMMETRIC"] = gen_in[gen_in.STOR.==2,:R_ID] | |||
STOR_ASYMMETRIC = gen_in[gen_in.STOR.==2,:R_ID] |
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.
Why do you do this for STOR_ASYMMETRIC and STOR_ALL but not STOR_SYMMETRIC? Also this is separate from the purpose of this PR, correct? That's fine if so but wanted to check.
invalid_newbuild = df.New_Build.==-1 | ||
if any(invalid_newbuild) | ||
@info "Deprecated '-1' entries in the 'New_Build' column. |
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.
Suggest throwing an error (or at least strong warning) instead of an @info if users do have the Can_Retire column.
which are -1 (indicating the ability to be retired). This input format is deprecated | ||
and may be removed in a future version. Instead, use a column 'Can_Retire', | ||
with entries of either 0 or 1. New_Build should be similarly restricted to {0,1}." |
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.
Can we reference the documentation 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.
See comments for suggestions. I have not stress tested personally but I will once the comments are addressed.
65824ee
to
eeffd3e
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.
See comment on info statement.
retirable = df[df.New_Build.!=-1, :R_ID] | ||
if !isempty(retirable) | ||
@info "The generators input file, 'New_Build' column, has some entries |
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 info message is not accurate. The "retirable" set which triggers the warning refers to resources with New_Build not equal to -1, which is inconsistent with the warning message which complains about resources with New_Build equal to -1. Perhaps we can just throw a more generic warning message about not having the Can_Retire column if this column is not found? And remind users of the correct input format? (What 0/1 means in each column)
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.
Plz lmk if this is satisfactory.
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.
Putting my Slack comments here for record keeping purposes: I think that we should require the Can_Retire column, and throw the info message anytime it is not present. Otherwise, it will not be possible to change the default Can_Retire value to 0 (which I would assume we would want if this column is optional) without breaking backward compatibility.
Notes from the meeting today: |
Previously this was one column "New_Build" which could be 0, 1, or -1. Now, New_Build should be either 0 or 1 and Can_Retire should be either 0 or 1.
5b64023
to
190dcac
Compare
if deprecated_newbuild_canretire_interface(df) | ||
retirable = df.New_Build .!= -1 | ||
else | ||
retirable = df.Can_Retire .== 1 | ||
end | ||
return Set(findall(retirable)) | ||
end | ||
|
||
function resources_which_can_be_built(df::DataFrame)::Set{Int64} | ||
buildable = df.New_Build .== 1 | ||
return Set(findall(buildable)) | ||
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.
These are the key functions.
@gmantegna This is complete now. Backward compatible (with warning) ✔️ I suppose for safety we might want to add a check that if you're using the new interface there should be only 1 or 0 in New_Build. Updated Example cases ✔️ . |
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!
Change the New_Build column to accept only {0, 1}; add a new required column Can_Retire which handles that feature separately (also {0, 1}). The change is backward-compatible, with a warning to update. --------- Co-authored-by: Gabe Mantegna <[email protected]>
This addresses #391 .