-
Notifications
You must be signed in to change notification settings - Fork 46
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
PEM BOP Functionality #348
Conversation
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 pretty good. A few minor changes. Thanks!
- distributed_peripheral_power > 0, | ||
hopp_results["combined_hybrid_power_production_hopp"] | ||
- distributed_peripheral_power, | ||
hopp_results["combined_hybrid_power_production_hopp"] |
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.
595 should be zero.
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, good catch. I do wonder why the tests didn't catch that?
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.
We must not have a test sensitive to it, which is concerning.
file_path = os.path.dirname(os.path.abspath(__file__)) | ||
|
||
|
||
def calc_efficiency_curve(operating_ratio, a, b, c, d): |
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.
Do we have a source for this equation?
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 see it in the README now, but I think putting in the reference and referring the reader to the markdown file in the doc string would be helpful.
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 reference and referred reader to markdown.
Returns: | ||
energy_consumption_bop_kwh (list or np.array): Energy consumed by electrolyzer BOP in kWh. | ||
""" | ||
from greenheart.tools.eco.electrolysis import get_electrolyzer_BOL_efficiency |
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 think this import would be better at the top of the 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.
Adjusted. It was previously a circular import but I moved the tool import to the top and added a comment.
@@ -32,6 +32,8 @@ | |||
PEM_H2_Clusters as PEMClusters, | |||
) | |||
|
|||
from greenheart.simulation.technologies.hydrogen.electrolysis.PEM_BOP.PEM_BOP import pem_bop |
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.
Let's keep import at the top of the 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.
This one is at the top of the file but I did adjust the other one
@@ -0,0 +1,45 @@ | |||
from pytest import approx, fixture |
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'd like to see one more test with the BOP in a system to make sure it is behaving as expected. Could be a quick small system.
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.
Okay I added a small integration test for the greenheart system. Let me know if that works
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 now
PEM BOP Model
This allows users to include electrical balance-of-plant (BOP) in modeling the PEM electrolyzer. The electrical BOP includes a transformer and a rectifier. The model calculates a curve fit based on the provided CSV it also limits the calculated efficiencies to the maximum and minimum operating ratio values in the CSV, making the overall function a piecewise implementation.
Related issue
Impacted areas of the software
Electrical BOP is calculated at each timestep.
ECO
tools andrun_simulation()
in greenheart_simulation.py are updated to reflect changes in calculation.Extracted some PEM electrolyzer functions into
PEM_tools.py
to remove circular import errors.Updated
plant_sizing_estimation.py
imports to usePEM_tools
. Created #351 since no tests exist for this functionality.Additional supporting information
Test results, if applicable
Added a test for BOP functionality.