Closed jmaguire1 closed 5 years ago
@Hayden-Reeve @emayhorn This one has "waiting for response" label since 2 weeks ago. Anyone working on it or has any update?
@jmaguire1 any idea when the updates will be complete?
@emayhorn @Hayden-Reeve Definitely still some things left to fix, I wouldn't try to test just yet. We can discuss some of my remaining issues tomorrow afternoon, I should be able to keep working on this most of tomorrow morning
@emayhorn @Hayden-Reeve currently recovering from a flu like bug I caught over the weekend, working on this now but that's delayed things by a few days :(
FYI @emayhorn @Hayden-Reeve I just committed some major changes to this, I have a few issues to fix tomorrow with the results but I've run a few of the services successfully myself
@jmaguire1 (cc: @hlngo , @emayhorn )
Thanks for the update. Once you have committed the fixes today we will test. Since we are running low on funds we want to minimize the number of tests and will wait until then.
@Hayden-Reeve (cc: @hlingo, @emayhorn) no problem. I'm trying to run most of the services myself as well, so I'll post some results as they come in. 1 out of 2 bugs fixed so far today.
@jmaguire1 , great - that will be a big help.
@jmaguire1 Please let us know when this is ready for testing and potential integration. We need to have the last call for updates today or tomorrow.
@Hayden-Reeve working on this right now. These are the results I'm currently getting:
I'm trying to figure out why it looks like the water heaters are frequently overresponding, most of my code related to this looks identical to what HVAC does in terms of dispatching the water heaters.
The underresponding is reasonable to me, that would just mean no water heaters are available to provide service. The overresponse seems like a bug, but it seems to be cycling between matching the request and having 1 extra water heater respond.
@jmaguire1 (cc: @emayhorn @afernandezcanosa )
This looks similar to the behavior of the EV fleet. Based on that learning it is likely based on discretization limits. Increasing the number of subfleets, or scaling the service size up may reduce this discretization error.
@Hayden-Reeve these are the results when I increase the fleet size from 10 water heaters to 500 (which is likely overkill, I'm guessing somewhere on the order of 100 is appropriate)
Still overshooting the request, so it looks like there's something going on beyond the discretization.
@jmaguire1
Please also check your Power sign convention. For loads P_base and P_togrid should be negative.
The P_service is only 1% of the P_base so I wonder if a larger request is needed to get out of signal to noise issues.
@Hayden-Reeve thanks, fixed the sign convention. I think this is ready for you to test: it looks like for the test case I was running the WH is providing the correct levels of service.
Also FYI I'll be on vacation starting tomorrow through the holiday, I'll miss tomorrow's call but if something urgent comes up you can either email me or post a note on GitHub and I'll see it.
@jmaguire1 ,
Thanks. Any way you could quickly review (and hopefully resolve) the merge conflicts for WH_fleet control and wh.py? (I am less worried about fleet_request as we should be using the version in the main src folder.)
Regards Hayden
@Hayden-Reeve (cc @hlngo) don't have the option to resolve conflicts (I think only Hung can?), or I would. To resolve the conflicts I'd just take everything in the new file
@jmaguire1 Can you delete the file: "src/fleets/water_heater_fleet/wh.pyc", recommit, and make another pull request on top of this pull request. Also, if you reference files correctly, you should reference the file "src/fleet_request.py" and shouldn't need "src/fleets/water_heater_fleet/fleet_request.py".
@jmaguire1 (cc: @hlngo , @emayhorn , @DavidWiniarski-pnnl )
I have tried to run the water heater code and have run into a number of problems. I am not sure how you achieved the response above given the issues. I am pretty sure I pulled your latest code and addressed the conflict issues but I may have made a mistake. Here are the issues - please address them when you are back from vacation:
WH_fleet_control does not seem to have capability to handle P_request = None
. When I tried to run reserve service the code crashed at:
https://github.com/GMLC-1-4-2/battery_interface/blob/f66765da08daeb689de1328edc61779259f2a11c/src/fleets/water_heater_fleet/WH_fleet_control.py#L144
When running regulation (which does not have any None
inputs the water heater models did not have the assigned_service_kW class required to scale the fleet. (This is tested for in the test_unit.py file):
When running the water heater device through test_unit.py it was not able to set up the fleet.
Given all these issues I am not sure that I have successfully pulled your latest code or that you have committed it successfully. I recommend that you run your code on the test_unit.py and scr/test.py files with a clean clone and the water heater changes (deleting the local fleet_request file) and submit a new PR.
Regards Hayden
@Hayden-Reeve @hlngo I've been on vacation the past week, but I'm working on this today. I'll tag you both when I have updates
@Hayden-Reeve: #1 should have been fixed in my last commit
@Hayden-Reeve: You don't see this in the code you've pulled down?: https://github.com/jmaguire1/battery_interface/blob/f68e5892a90c69c57f511389c6b54d00bcca8e11/src/fleets/water_heater_fleet/wh_fleet.py#L665
@Hayden-Reeve: There's always a possibility that this was a merge issue, unfortunately I don't think I can resolve conflicts unless Hung merges, but I think you should be able to accept the whole new file for the conflicts rather than trying to pick things manually line by line
@jmaguire1
The code I pulled down has this line. I think the issue is that fleet_factory sets _fleet
to be WH_fleet_control not WH_fleet so the command self._fleet.assigned_service_kW()
sends it looking in the wrong place. Please try to run reserve or regulation service from the main integration test file and see if it can run.
@Hayden-Reeve Of course! I forgot to add/commit some of the testing files (some of which I changed to call WH_fleet instead of WH_control_fleet). I just pushed those changes into this pull request.
@Hayden-Reeve: Just reran reserve service and got the same results as before:
Regulation is running now, I'll keep you posted how that one and artificial inertia look.
@Hayden-Reeve: Artifical Inertia results (when I run this service, I get an error "[Errno 2] No such file or directory: 'C:\battery_interface_jmaguire\src\integration_test\artificial_inertia\20190528_ArtificialInertia_WaterHeater.png'", but the plot prints, perhaps it ought to be saved at an earlier step?):
Regulation results (2 second, dynamic):
I haven't thought through exactly what the results SHOULD look like, but both cases appear to run without error. Hopefully you were able to get things to run successfully on your end with the files I added, if not let me know and I can make a new pull request.
@jmaguire1
You will need to manually create the folder src\integration_test\artificial_inertia to get around the AI bug. You may also want to increase the fleet scaling to see if that helps the results. I will run this afternoon. I think a new PR built on the freshest master would be good as there are a lot of files committed that may not mean to be updated. I am concerned that the changes to the core src files will affect other devices.
@Hayden-Reeve
No problem, I'll make a new clean pull request. I don't think I changed anything that would affect the other devices, but I did change the main fleet file name to be "WH_fleet" (which to me reads more cleanly), which required changing things like the fleet_factory. Definitely let me know if you run into issues this afternoon.
@Hayden-Reeve: Artifical Inertia results (when I run this service, I get an error "[Errno 2] No such file or directory: 'C:\battery_interface_jmaguire\src\integration_test\artificial_inertia\20190528_ArtificialInertia_WaterHeater.png'", but the plot prints, perhaps it ought to be saved at an earlier step?):
Regulation results (2 second, dynamic):
I haven't thought through exactly what the results SHOULD look like, but both cases appear to run without error. Hopefully you were able to get things to run successfully on your end with the files I added, if not let me know and I can make a new pull request.
Hi Jeff, could you plz mark the lines you modified the frequency response part? Just to expedite my debug process.
@Hayden-Reeve: Artifical Inertia results (when I run this service, I get an error "[Errno 2] No such file or directory: 'C:\battery_interface_jmaguire\src\integration_test\artificial_inertia\20190528_ArtificialInertia_WaterHeater.png'", but the plot prints, perhaps it ought to be saved at an earlier step?): Regulation results (2 second, dynamic): I haven't thought through exactly what the results SHOULD look like, but both cases appear to run without error. Hopefully you were able to get things to run successfully on your end with the files I added, if not let me know and I can make a new pull request.
Hi Jeff, could you plz mark the lines you modified the frequency response part? Just to expedite my debug process.
@ORNLJD Can you be a bit more specific about what you're looking for? What results are you getting, and which test are you concerned about?
@Hayden-Reeve (cc @hlngo)
When I try to make a new pull request, it doesn't look like I'm able to since I'd basically be exactly copying this pull request. I instead get an option to view this pull request when I try to make a new one. Am I doing something wrong? Here's what I see on GitHub:
I suppose I could try closing this pull request, THEN making a new one, but since things would basically be identical I'm not sure how much that would help you out...
@Hayden-Reeve: Artifical Inertia results (when I run this service, I get an error "[Errno 2] No such file or directory: 'C:\battery_interface_jmaguire\src\integration_test\artificial_inertia\20190528_ArtificialInertia_WaterHeater.png'", but the plot prints, perhaps it ought to be saved at an earlier step?): Regulation results (2 second, dynamic): I haven't thought through exactly what the results SHOULD look like, but both cases appear to run without error. Hopefully you were able to get things to run successfully on your end with the files I added, if not let me know and I can make a new pull request.
Hi Jeff, could you plz mark the lines you modified the frequency response part? Just to expedite my debug process.
@ORNLJD Can you be a bit more specific about what you're looking for? What results are you getting, and which test are you concerned about?
@jmaguire1 Thank you, I will ping you through separate emails.
@Hayden-Reeve (cc @emayhorn @hlngo)
I'll point out that the conflicting files seem to be with previous versions in this pull request, NOT with master. I diffed the conflicting files that are common to all the fleets on this branch with what's in master, here's the result:
fleet_request:
fleet_factory:
test:
test_unit:
service_factory:
The changes in service_factory are the only ones that I think are substantial, I was having trouble creating the fleets because the sys.path wasn't adding this directory. It looks like in a lot of files they're trying to add C:/ to the sys.path, but I was having issues with some of the files I needed not being added to the path. It's ugly to have it hard coded, maybe you have a better idea of how to add this to the path?
As a minor side note, it looks like a lot of the other fleets are always adding C:/ to the sys.path, even if it's already there. It doesn't cause any problems, but does lead to an ugly sys.path...
@jmaguire1 (cc: @emayhorn @hlngo @DavidWiniarski-pnnl )
Thanks for the update. @hlngo can recommend the best way to resubmit. I think if you cancel this PR, clone a new master then add those changes in and submit a new PR only the real changes will show up.
Note that I also had to make some import references in wh_fleet.py full paths to address what I think is a PyCharm issue:
from fleets.water_heater_fleet.load_config import LoadConfig
from fleets.water_heater_fleet.wh import WaterHeater
I applied the changes to fleet_factory and the code ran successfully for reserve service (and peak service). However, for reserve service a strange phenomenon occurred as I explored different magnitudes for scaling factor. As I increased scaling factor from 5 to ~30 the baseline performance changed. This should not happen and the baseline should be the same for both runs.
This did not seem to happen for peak management service (see below). I wonder if this is because one is looking for an increase in load and one is looking for a decrease? I am also concerned about the dynamic response of these two cases. For reserve (dT = 1 min) the transient drops from -100 MW to -20 MW in about 2 hours. For peak management (dT = 60min) it occurs in about two days. Note that we are under the impression that the maximum time step that water heaters can run at is 60 minutes. However when I run it at 15 minutes and 5 minutes the results (bottom two plots) indicate that the model does not achieve time-step independence. Is there a minimum time step we should be using? These results give me serious concern about the validity of the model.
@Hayden-Reeve (cc @emayhorn @hlngo @DavidWiniarski-pnnl)
Thanks for running and testing this! Agreed that the baseline shouldn't change, I'll look into why that's happening but it should be independent of the scaling factor.
As for timestep independence, my first thought is that it's possible the draw profile isn't being correctly scaled at different timesteps. I made some pretty recent changes to that, so I'll start by looking at that. We're providing a 1 minute draw profile, which should be either spread out over multiple timesteps (if timestep<1 minute) or "smoothed" to the timestep (if timestep>1 minute). One of the things I added is an output file that gets created for each WH ("WH_fleet_output.csv). Do you happen to have that for any of the cases you ran? If not I can regenerate it pretty quickly, it's helpful for debugging potential physics issues like this.
I have not been saving those for each case. Here is the latest one.
From: jmaguire1 notifications@github.com Sent: Tuesday, May 28, 2019 3:21 PM To: GMLC-1-4-2/battery_interface battery_interface@noreply.github.com Cc: Reeve, Hayden M hayden.reeve@pnnl.gov; Mention mention@noreply.github.com Subject: Re: [GMLC-1-4-2/battery_interface] Latest updates to the water heater model (#70)
@Hayden-Reevehttps://github.com/Hayden-Reeve (cc @emayhornhttps://github.com/emayhorn @hlngohttps://github.com/hlngo @DavidWiniarski-pnnlhttps://github.com/DavidWiniarski-pnnl)
Thanks for running and testing this! Agreed that the baseline shouldn't change, I'll look into why that's happening but it should be independent of the scaling factor.
As for timestep independence, my first thought is that it's possible the draw profile isn't being correctly scaled at different timesteps. I made some pretty recent changes to that, so I'll start by looking at that. We're providing a 1 minute draw profile, which should be either spread out over multiple timesteps (if timestep<1 minute) or "smoothed" to the timestep (if timestep>1 minute). One of the things I added is an output file that gets created for each WH ("WH_fleet_output.csv). Do you happen to have that for any of the cases you ran? If not I can regenerate it pretty quickly, it's helpful for debugging potential physics issues like this.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/GMLC-1-4-2/battery_interface/pull/70?email_source=notifications&email_token=ALGDK67BW3Z3TX62ZSSXYT3PXWV3ZA5CNFSM4HBP6O52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWNTQUA#issuecomment-496711760, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ALGDK64VGW3XOQXTFPLHWCLPXWV3ZANCNFSM4HBP6O5Q.
@Hayden-Reeve (cc @emayhorn @hlngo)
I think I figured out the issue where timesteps were having an undue influence on the results: I was initializing a timestep, but wasn't updating it with what was passed in to the test file. So everything was running with a 1 minute timestep, even when an hour was requested, which is why it would take up to 2 days for things to settle. Here's some updated results:
60 minute timestep:
5 minute timestep:
Things appear to be averaging around the same value, but the hourly results look a lot noisier. I think that's somewhat to be expected, as with an hour timestep if the electric resistance comes on it's likely to stay on until the tank is fully heated (since it's trying to figure out how much heat is added to meet the setpoint at the end of the hour). With a 5 minute timestep, the water heater is less likely to fully heat during the timestep, and there's also more timesteps where it can just be losing heat.
I still need to check why the baseline case changes with the fleet scaling, I'll ping you again once I've got that sorted as well.
I also added in the energy impact metrics (copied directly from the HVAC model).
@hlngo: Hayden asked me to create a clean pull request for this, but if I try to make a new one using all of this code it just shows me a link to this repo (there's a screenshot in the conversation above). What would be the best way to make a clean pull request?
@Hayden-Reeve
Probably a bit of ignorance on my part, but when you talked about adjusting the scale factor, which variable specifically were you changing? Working now on replicating your results so that I can fix the issue
@jmaguire1
It is the serviceweight in the config.ini file:
@Hayden-Reeve that's what I thought. There's a comment in there (from someone else's file) that the service weight should be in the range 0-1, but you were tweaking it to 5 and 30? I definitely agree that shouldn't break the model, but maybe I'm not understanding exactly how the service weight should be applied.
@jmaguire1
ServiceWeight is just used by assigned_service_kw to linearly multiply the service request:
I have been altering it by manual iteration to find a reasonable weighing for each combination of device and service (since each service has a different request magnitude and each device a different fleet rating). If the service weighting is too low the request is in the noise (1-2% of the fleet rating) and if the weighting is too high the fleet can not meet the request and is penalized. not sure why we have to turn WH weighting up so high but it does not give me heartburn.
@Hayden-Reeve I see, so long as it's not concerning you I'm OK with it. Digging into why the baseline is changing now that I understand why we're setting things so high.
@Hayden-Reeve
I've been thinking a bit more about the baseline performance. For each water heater and each timestep, we first calculate the baseline performance, process requests for service, then update the tank temperature AFTER it's provided services. If the WH had provided services last timestep (say adding load), the water heater might already be at the setpoint because it was adding load. When you scale the service weight, you're also increasing the amount of load add asked for (in this particular case), so more of the water heaters are likely to be at setpoint, which means the baseline performance should change. Am I thinking about this incorrectly? Should we be calculating the baseline performance without factoring in past service requests?
@jmaguire1
The baseline performance is the equivalent entire drive cycle performance with no grid service request, not just the next step in the analysis. So for reserve service it would be the baseline performance for two days of operation with no service requests. I hope this helps. I am free this afternoon if we need to talk more.
@Hayden-Reeve
That's very helpful, I can make that change but wanted to make sure I was thinking about things correctly. One more question for you: in the reserve service what's plotted as P_base is df_1m['P_togrid'] - df_1m['Response']
After I made some changes, I'm looking at P_base as reported by the water heater model and it's independent of the service weight, but that doesn't show up in the figures. Is there a reason why this service doesn't just plot P_base as reported by the fleet?
@jmaguire1
This is a hold over in LBNL's code from the time when devices did not report P__base. Since it these are equivalent (by definition P_response = P_togrid - P_base) they should be identical to updating it to plot P_base.
@Hayden-Reeve
OK, I figured the fact that they weren't identical to mean that there was still an issue in my code, thanks for the history. The fact that what's plotted doesn't match what I'm calculating doesn't equal P_base means there's definitely still an issue, working on a fix now.
@Hayden-Reeve
I think I fixed most of the issues, but I'm still thinking about the sign convention. Here are the latest results (I pretty much copied the sign conventions from HVAC):
The reason why P_base was being changed during the requests is due to a sign error I had previously. If I recall correctly, power provided TO the grid should be negative, so the bottom figure should basically be reversed, correct?
@jmaguire1
Power supplied to the grid is positive. Loads (power from the grid) is negative. I will send you a copy of the device report that defines this. The rest of the trends look good.
@Hayden-Reeve
Right, that's what I was expecting. So for this reserve service, the request is actually a load reduction, not addition, correct?
Updates that I've made to the water heater model. Includes sign change and output files for each device.