search

GMLC-1-4-2 / battery_interface

Implemenation of Device Models and their Battery Equivalent Interface
MIT License
5 stars 0 forks source link

Waterheater update #132

Closed Hayden-Reeve closed 5 years ago

Hayden-Reeve commented 5 years ago

@hlngo (cc: @emayhorn @DavidWiniarski-pnnl @yliu250 @raselmahmud02 )

This a clean PR for the water heater. We may have a few additional changes today. Please merge it after COB today and reject #70. Any changes after that can be in a clean new PR.

jmaguire1 commented 5 years ago

@Hayden-Reeve (cc: @hlngo @emayhorn @DavidWiniarski-pnnl @yliu250 @raselmahmud02 )

Awesome, thanks! If I have any more changes I'll put them here.

jmaguire1 commented 5 years ago

@Hayden-Reeve (cc @hlngo)

If we reject #70, do things get archived somewhere or is it permanently deleted? A lot of the discussion on that PR might be useful to refer to as an archive.

Hayden-Reeve commented 5 years ago

@jmaguire1

To be more careful with my language - it will be closed and still visible under the 'closed' tab.

jmaguire1 commented 5 years ago

@Hayden-Reeve

Thanks, that's what I thought but I wanted to confirm.

jmaguire1 commented 5 years ago

@Hayden-Reeve

Excuse my GitHub ignorance, but what's the best way to pull down this pull request locally so I can make changes to it? Usually when I work on GitHub projects I make a new branch for each pull request and then work on that particular branch, but I know this project is using a different workflow

emayhorn commented 5 years ago

@jmaguire1 Hung has given instructions here on how to checkout a pull request locally: https://github.com/GMLC-1-4-2/battery_interface#how-to-test-a-pull-request

jmaguire1 commented 5 years ago

@emayhorn

Thanks, I missed those when I was looking in the readme.

Hayden-Reeve commented 5 years ago

@jmaguire1

I am not smart enough to know if or how you can contribute to this pull request. I may have to add to it for you or we can just have this merged then start a new contribution from you (built on a clean clone).

jmaguire1 commented 5 years ago

@Hayden-Reeve (cc: @emayhorn @DavidWiniarski-pnnl @yliu250 @raselmahmud02 )

@emayhorn was able to show me how to add on to this pull request. I did just push a commit for a sign issue that hadn't come up in the regulation service. Here's the latest:

Results from Reserve: image

Results from Regulation (Traditional): image

Results from Regulation (Dynamic): image

I also tried to run artificial inertia, but in test.py I'm hitting an Exception "Need to integrate time step defaults for AritificalInertia". I'm pretty happy with the results I'm getting for the services I tested, but if anyone thinks they look suspicious let me know and I can dig in. I don't know if I can easily add my changed file, so I'll email it to Hayden directly.

ORNLJD commented 5 years ago

@jmaguire1 Hi Jeff, I think I saw similar issues for the baseline outputs are not matching in the Dynamic and Traditional regulation services though they are simulated for the same time period. For your case, shouldn't the baseline always be non-positive?

Hayden-Reeve commented 5 years ago

@jmaguire1

Any idea what may be causing EWH P_base to be positive for regulation services? We are trying to debug issues with regulation so it may be service related.

jmaguire1 commented 5 years ago

@Hayden-Reeve (cc @ORNLJD )

Not offhand, but I can dig into it this afternoon. I agree that P_base should always be negative.

jmaguire1 commented 5 years ago

@Hayden-Reeve (cc @ORNLJD @DavidWiniarski-pnnl)

I'm not seeing any positive P_base values being reported by wh.py or wh_fleet.py when I run with regulation services. Is it possible that this is related to the service (or something is being plotted incorrectly)?

Hayden-Reeve commented 5 years ago

@jmaguire1

Thanks for checking - May be an issue with regulation plotting.

jmaguire1 commented 5 years ago

@Hayden-Reeve

No problem, I can keep digging into what's being plotted and try to figure out what exactly is happening here. I'm slightly nervous that since P_base itself isn't what's being plotted there might be an issue with my device model, I'll let you know if I find anything that needs to be fixed.

Hayden-Reeve commented 5 years ago

@hlngo - please merge this PR