-
Notifications
You must be signed in to change notification settings - Fork 45
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
Uniform output #312
Uniform output #312
Conversation
…ert battery dispatch to kW
…nergy breakdown available from the post_process function outputs
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 have left a few comments for discussion, otherwise looks good.
@@ -818,7 +887,30 @@ def simple_solver(initial_guess=0.0): | |||
return hopp_results, electrolyzer_physics_results, remaining_power_profile | |||
elif config.output_level == 7: | |||
return lcoe, lcoh, steel_finance, ammonia_finance | |||
|
|||
elif config.output_level == 8: |
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.
What do you think about getting rid of output_level
completely and just returning the data class? I know that it might mess a few folks up right now but lots of other breaking changes are happening on this branch and I think using data classes will help improve the greenheart simulation generalization
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 would like to avoid running post-processing and passing a lot of extra stuff around during optimization runs. So, for now anyway, I would like to keep the output levels. We should probably make output level 8 the de facto in our example yamls though to encourage people to use that option
if not os.path.exists(filepath): | ||
os.makedirs(filepath) | ||
|
||
df.to_csv(os.path.join(filepath, "power_series.csv")) | ||
df.to_csv(os.path.join(filepath, "energy_flows.csv")) |
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 as we keep generating more figures and data files, we should try to settle on what sort of information should be included in the file name. This format only works in the single run case which can make running sweeps difficult
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 agree, data and figure handling is a mess and should be cleaned up, unified, and generalized.
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 should be made an issue so we can address it, but not in this PR
tests/greenheart/test_openmdao.py
Outdated
hopp_config_steel_ammonia_filename = Path(__file__).absolute().parent / "input_files" / "plant" / "hopp_config.yaml" | ||
greenheart_config_onshore_filename = Path(__file__).absolute().parent / "input_files" / "plant" / "greenheart_config_onshore.yaml" | ||
turbine_config_filename = Path(__file__).absolute().parent / "input_files" / "turbines" / "osw_18MW.yaml" | ||
floris_input_filename_steel_ammonia = Path(__file__).absolute().parent / "input_files" / "floris" / "floris_input_osw_18MW.yaml" | ||
rtol = 1E-5 | ||
|
||
class TestBoundaryDistanceComponent(unittest.TestCase): |
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.
Rewrite as pytests?
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.
Right, good catch. I told you I would get this into this PR and forgot. I'll try to get to it soon.
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 have adjusted the test style in test_openmdao.py
to follow the function pytest approach
Uniform flow output
This pull request fixes a bug where not all flows in the system were of uniform length. It also provides a file output with all electricity and hydrogen flows. I also took the opportunity to make a new file for testing the greenheart utilities and moved the greenheart system test out of the test_hydrogen folder and up to the top-level greenheart test directory to match the package file structure.
Another, larger change in the PR is the addition of the GreenHEARTOutput data class. Hopefully this data class will eventually take the place of the different output levels (though we may keep some of the output levels for optimization purposes).
Related issue
This PR may close, or get close to closing, issue #283
Impacted areas of the software
greenheart_simulation and greenheart testing
Additional supporting information
Test results, if applicable