-
Notifications
You must be signed in to change notification settings - Fork 41
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
Syncing GreenSteel and GreenHEART features #322
Syncing GreenSteel and GreenHEART features #322
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.
This looks good to me once we get the keys changes in greenheart_simulation figured out.
I sent a PR into your branch that fixes the keys check.
Also, we need to add the h2_itc values to the reference plant input files
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.
Looking pretty good. There are some fairly major comments:
- We need to avoid saving outputs into the config dict or class since the config is persistent across analyses when running an optimization or DOE.
- Many test values changing unnecessarily. I think we should just change the GreenSteel tests and the optimization in openmdao tests to use variable sizing and leave the rest as they were.
...ples/reference_plants/05-offshore-hydrogen-ca/input/plant/greenheart_config_offshore_ca.yaml
Show resolved
Hide resolved
@@ -294,6 +298,7 @@ def run_h2_storage( | |||
hydrogen_storage_capacity_kg, hydrogen_storage_duration_hr, hydrogen_storage_soc = hydrogen_storage_capacity(electrolyzer_physics_results['H2_Results'], greenheart_config['electrolyzer']['rating'], hydrogen_storage_demand) |
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 it looks like we have two separate sets of logic around sizing, pressurized tower and demand/type. I think we should combine these sets of logic into a single if/else set starting with type for zero storage capacity, then checking demand, from max turbine storage, and finally using hour and max fill rate as default if the others are not specified. Eventually this should have the sizing type as a variable rather than potentially conflicting inputs, but for now we can at least simplify/clarify the logic
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've changed the logic so that we have a single if/elif/else statement around sizing and then a signle if/elif/else statement around h2 storage technology type. I moved the logic around if on a turbine to inside the specific technology (pressure vessel and turbine) and included a check up front for illegal technology choices based on turbine location selection.
@@ -81,7 +81,7 @@ def test_simulation_wind(subtests): | |||
|
|||
with subtests.test("lcoh"): | |||
assert lcoh == approx( | |||
7.056407257515502 | |||
7.248875250895419 |
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'm inclined to use the fixed sizing for these tests and just use the variable sizing in the GreenSteel tests so we avoid changing test values and keep some tests for the fixed storage size approach
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.
These value changes are coming from using electrolyzer_physics_results['H2_Results']['Time Until Replacement [hrs]']
rather than the time_between_replacement
for the electrolyzer schedule, not changing hydrogen sizing.
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 looks good to me
Updating GreenHEART features based on GreenSteel project code
electrolyzer_physics_results['H2_Results']['Time Until Replacement [hrs]']
for electrolyzer replacement schedule. Added depreciated warning fortime_between_replacement
variable in greenheart_config.costing_general_inflation
to adjust to correct cost year.land_cost
.Related issue
Impacted areas of the software
Additional supporting information
Updated additional greenHEART tests based on new features and bug fixes.
Test results, if applicable
Several tests in the
test_greensteel.py
file are commented out. They will fail if uncommented and they are left commented out to illustrate the differences between GreenSteel and GreenHEART frameworks.test_greenheart_system.py
has updates to lcoh and lcos values because of updating financials to useelectrolyzer_physics_results['H2_Results']['Time Until Replacement [hrs]']
for electrolyzer replacement schedule. Added depreciated warning fortime_between_replacement
variable in greenheart_config.