-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adding solar controller and replacing AMR-Wind standin with FLORIS standin in example 07 #104
Conversation
…solar-pysam tried to push to origin solar-pysam but wasn't able to because this branch was behind the remote by a commit
Merging develop into local branch to prepare for PR back to NREL develop
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.
No big changes, just the path leading to the output file and a change in an output variable name
max-power-prediction-OK-pysam.ipynb
Outdated
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.
Is this file supposed to be included with 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.
Nope, deleting this. Thanks.
print("sim_time_s = ", inputs["py_sims"]["inputs"]["sim_time_s"]) | ||
sim_timestep = int(inputs["py_sims"]["inputs"]["sim_time_s"] / self.dt) | ||
# sim_time_s = inputs["py_sims"]["inputs"]["sim_time_s"] | ||
sim_time_s = inputs["time"] |
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 change (from inputs["py_sims"]["inputs"]["sim_time_s"]
to inputs["time"]
is causing the test_step
test in tests/solar_pysam_test.py to fail. Possibly, the step_inputs
dictionary in tests/solar_pysam_test.py just need to be updated to add the "time" field?
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.
Fixed! Thanks for pointing this out
|
||
controller: | ||
|
||
external_data_file: solar_power_reference.csv |
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 file is missing from the repo, I think
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.
Added!
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 file is maybe not used?
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.
In the batch_script
, I wanted to include the option to run hercules without the pv controller, which uses hercules_input_000.yaml
as well as the option to run with the pv controller, which uses hercules_controller_input_000.yaml.
So left both files in there. Is that ok?
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.
README updated to clarify these options
Hi @brookeslawski! I left a handful of small comments, but perhaps I'm a little confused about the files in the 07_floris_standin_and_solar_pysam. It seems that there is a bit of a mix in there of files that are needed to run this example, and files that maybe are just carry-overs from when it was 07_amr_wind_standin_and_solar_pysam, several of which I think are no longer used. Did you mean to leave those in? Is the intention that the user could run the example with the AMR-Wind standin, as well as with the FLORIS standin? |
…solar-pysam tried to push, but said there were changes in remote that weren't local so now i'm pulling.
times = np.arange( | ||
self.helics_config_dict["starttime"], self.helics_config_dict["stoptime"], self.dt | ||
self.helics_config_dict["starttime"], | ||
self.helics_config_dict["stoptime"]+(2*self.dt), |
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.
Changed this to create times
variable that includes the stoptime + one more timestep. np.arange
is half-open, which does not include end
and thus was causing errors when hercules was looking for a setpoint at the last timestep, but not finding it even though it was in the input external_data_file
Hi @misi9170 - Thank you for the review. I've deleted the amr_wind_standin files, which was just the |
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 for addressing my comments, @brookeslawski ! I've been able to run the example in both controlled and uncontrolled configurations. I made a couple of small changes, hopefully you're happy (but feel free to revert if not):
- Removed one of the
&
s in batch_script.sh, because I found this was causing the script not to terminate properly (and it was my fault that that went in in the first place!) - Added handling to the test_read_output.ipynb notebook for when running the uncontrolled case
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! I left a few comments that don't really need to be addressed, just some thoughts. Otherwise, good to merge!
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.
Just noticing that this file is really long. Do we need all of it, or can we shorten it for example purposes?
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 do have a five-timestep version of this file for testing, but I left the longer one in there to be able to demonstrate more of a variation in the setpoint signal. It doesn't take long to run, and you can always change the stoptime
to be something shorter for quicker tests. The input stoptime
doesn't need to match the time in this file.
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 just edited the results plot so that all of the units were consistent. I don't know if you can see the changes on github though.
1. Solar controller
Implementing a simple PV plant controller that trims any PV plant power output above the setpoint at each timestep. This is similar to what
pysam
does. The "curtailment" is applied uniformly to the entire PV plant.Requires an input power setpoint signal, which is read in through the input file field
external_data_file.
In the future, this power setpoint might be calculated internally in
hercules
and this controller will need to be modified to handle that input.2. FLORIS stand-in
The AMR-Wind stand-in was causing the errors shown below.
FLORIS was determined to be a better stand-in anyway, and was easy to implement, so the FLORIS stand-in replaced the AMR-Wind stand-in.