search

GMLC-1-4-2 / battery_interface

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

Fixed sign conventions and API variables for Electrolyzer and FuelCell #63

Closed rkadavil closed 5 years ago

hlngo commented 5 years ago

@rkadavil Can you change the encoding of your ini files (for both Electrolyzer and Fuelcell) to be UTF-8?

Hayden-Reeve commented 5 years ago

@rkadavil , (cc: @hlngo , @yliu250 )

Electrolyzer is crashing at the line below for the Artificial Inertia service:

https://github.com/GMLC-1-4-2/battery_interface/blob/adf180950f36448007a843828dcd5e241de67f64/src/fleets/electrolyzer_fleet/ey_fleet.py#L152

This service does not provide a Preq (that is, Preq = 'none'). This device should include 'process request' and 'frequency_watt' methods to call 'get_frequency' to return the grid frequency at that time. Based on that frequency's deviation from 60 Hz it will then determine its own response and calculate a grid service. See for reference #52 , #53 .

rkadavil commented 5 years ago

@rkadavil , (cc: @hlngo , @yliu250 )

Electrolyzer is crashing at the line below for the Artificial Inertia service:

battery_interface/src/fleets/electrolyzer_fleet/ey_fleet.py

Line 152 in adf1809

self.ey_Ne = round(abs(Preq)/self.ey_E_size, 1) This service does not provide a Preq (that is, Preq = 'none'). This device should include 'process request' and 'frequency_watt' methods to call 'get_frequency' to return the grid frequency at that time. Based on that frequency's deviation from 60 Hz it will then determine its own response and calculate a grid service. See for reference #52 , #53 .

Electrolyzer model at present doesn't have frequency regulation or AI service implemented. We are still working on it. What we have implemented is the process_request method to accept the time-stamped Prequest inputs and provide a response based on the device physics.

rkadavil commented 5 years ago

Please review commit# 2464540. Included impact metrics in the Electrolyzer model. No service exists. We are working on it.

afernandezcanosa commented 5 years ago

@rkadavil Correct me if I am wrong, but it looks like you have committed changes in all the fleets (43 files changed) and I think you should be only committing changes in your fleet. Is this correct, @hlngo?

rkadavil commented 5 years ago

@rkadavil Correct me if I am wrong, but it looks like you have committed changes in all the fleets (43 files changed) and I think you should be only committing changes in your fleet. Is this correct, @hlngo?

Hey. Sorry about that. I downloaded a fresh clone from the main rep and made the commits only to our models. Please ignore this commit.

hlngo commented 5 years ago

@afernandezcanosa You are correct. @rkadavil Thanks for the PR. I will ignore unrelated files.

rkadavil commented 5 years ago

Hi @hlngo

@afernandezcanosa You are correct. @rkadavil Thanks for the PR. I will ignore unrelated files.

hi there. I've made changes to the Electrolyzer and FC models to include the impact metrics logging and forecast method that was missing. The commit# 3b4622c is for Electrolyzer and b052542 is for FuelCell.

rkadavil commented 5 years ago

Hi @hlngo Please review commit#9b3e6dc. The impact metrics method was updated to create the CSV file at the correct location for both the devices - Ey and FC.

rkadavil commented 5 years ago

Hi @hlngo Please review commit#9b3e6dc. The impact metrics method was updated to create the CSV file at the correct location for both the devices - Ey and FC.

I've updated both the models to fix the crash issue for P_req = None in commit # 3343b38 Please review and accept the changes.

hlngo commented 5 years ago

Reject the PR as @Hayden-Reeve talked to ANL about this one.

Hayden-Reeve commented 5 years ago

@rkadavil

As discussed please resubmit a clean PR and I can evaluate it. Hopefully that will address some of the issues and discrepancies we were seeing.