Integrate steel/ammonia models into run_simulation
#280
Conversation
We'll likely need to revisit some of these numbers to make sure we're asserting reasonable values
| ) | ||
|
|
||
| if "steel" in greenheart_config: | ||
| if verbose: |
There was a problem hiding this comment.
I'm inclined to say that the bulk of the code inside the if "steel".... statement should be in its own function to keep the simulation code cleaner.
There was a problem hiding this comment.
I agree, I'll move it out
| steel_finance = run_steel_finance_model(steel_finance_config) | ||
|
|
||
| if "ammonia" in greenheart_config: | ||
| if verbose: |
There was a problem hiding this comment.
Same comment as for steel
There was a problem hiding this comment.
I wonder about this changing the config dictionary. That feels a little dangerous. Let's discuss this in our code meeting.
There was a problem hiding this comment.
I agree. As it currently stands, this will overwrite some configs even if the user does not want that. I was thinking one option is to first check if the necessary field is user-provided, and calculate if not
There was a problem hiding this comment.
I think it may be best to create an output data class where outputs can be stored. We should probably have a separate entry in the Greensteel config for steel and ammonia to specify if the user provided LCOH or the calculated LCOH should be used so the knock-on effects of providing an LCOH at input are clear.
camirmas
force-pushed
the
greensteel-eco-sync
branch
from
50c3892
to
dcfbceb
Compare
This is likely to change as we refactor to use dataclasses for our simulation config, but for now, this will make sure that we dont: - override user configs for steel/ammonia - modify the config `dict` unnecessarily There's definitely a more refined interface solution that can accommodate scenarios where the user does/doesn't want capacity calculations to be run, but this should make the behavior a bit more predictable for now.
|
I added some measures to mitigate override issues, but I'm going to revisit this in more detail when I work on configs next. I think it's important to clearly define where we may be overriding certain config values as we go through the steel/ammonia model steps, and to allow users to opt out by providing their own values, where appropriate |
|
I think we can merge this as long as we have an issue made to address the config override problem more clearly. |
Integrate steel/ammonia models into
run_simulationblackautoformatter on simulationgreenheart_simulation.pytest_greenheart_systemto usepytestandsubtestsgreenheart_config_offshore.yaml)run_simulation.Considering refactoring into convenience methods that exist in their respective modules (i.e.(EDIT: This is complete)steel.py,ammonia.py)output_levelthat includes steel/ammonia finance outputs. I'm a bit unclear about the purpose behindoutput_levels, but it may be worth considering consolidating into an output dataclass withOptionalfields for easier use and documentationtest_greenheart_systemensuring that we get finance outputs. We'll probably need to come back to these to make sure the values make senseSimilar to HOPP, the YAML structure for greenheart steel/ammonia configs matches their respective input dataclasses (
CapacityModelConfig,CostModelConfig,FinanceModelConfig).For example:
Related issue
Impacted areas of the software
Additional supporting information
Test results, if applicable