Iron: Adding electrowinning capabilities#432
Iron: Adding electrowinning capabilities#432jmartin4u wants to merge 56 commits intoNatLabRockies:developfrom
Conversation
kbrunik
left a comment
There was a problem hiding this comment.
Thanks for working on this! Overall super cool and exciting to have some more functionality added to H2I. I know this isn't quite finished with regards to documentation and testing but thought I could share some initial thoughts.
elenya-grant
left a comment
There was a problem hiding this comment.
Hi Jonathan! I think this is super cool and the example is very well done! I didn't take a super deep look into the new models but provided some initial feedback (some of it may be irrelevant depending on other changes you plan on making). Would be happy to do a deeper review once it's ready! Thanks for the work on this! Very exciting!
h2integrate/converters/iron/test/test_humbert_stinn_ewin_cost.py
Outdated
Show resolved
Hide resolved
| self.add_output("labor_opex", val=0.0, units="USD/year") | ||
| self.add_output("NaOH_opex", val=0.0, units="USD/year") | ||
| self.add_output("CaCl2_opex", val=0.0, units="USD/year") | ||
| self.add_output("limestone_opex", val=0.0, units="USD/year") |
There was a problem hiding this comment.
shouldn't feedstock costs (lime, electricity, ore, etc) be taken care of with feedstock component(s)? In my mind, this should only output costs associated with this tech, not its feedstocks.
| elec_price = inputs["price_electricity"] | ||
|
|
||
| # Add ore transport cost TODO: turn iron_transport into proper transporter | ||
| ore_price += ore_transport_cost |
There was a problem hiding this comment.
ore transport is now a separate model, should not be included here.
|
|
||
| # Humbert Opex model - from SI spreadsheet (doi.org/10.1007/s40831-024-00878-3) | ||
| # Default costs - adjusted to 2018 to match Stinn via CPI | ||
| labor_rate = 55.90 # USD/person-hour |
There was a problem hiding this comment.
Should labor_rate be an attribute of the configuration class?
| labor_rate = 55.90 # USD/person-hour | ||
| NaOH_cost = 415.179 # USD/tonne | ||
| CaCl2_cost = 207.59 # USD/tonne | ||
| limestone_cost = 0 |
There was a problem hiding this comment.
I think these feedstock costs should be taken care of with the feedstock component.
elenya-grant
left a comment
There was a problem hiding this comment.
This is coming along nicely! Thanks for the work on this! I think the performance model is looking awesome, I have some remaining questions and comments on the cost model. Another small nit-pick (non-blocking) is that links to papers can be embedded in docstrings as hyper-links if they're formatted like this:
All of these values come from the SI spreadsheet for the Humbert paper that can be downloaded
at `doi.org/10.1007/s40831-024-00878-3 <https://link.springer.com/article/10.1007/s40831-024-00878-3>`_
Thanks for making a lot of the previously suggested changes. I think it's nearly ready! Let me know if you want to discuss any of the comments I left!
| ewin_type = self.config.electrolysis_type | ||
| # Look up performance parameters for each electrolysis type from Humbert Table 10 | ||
| if ewin_type == "ahe": | ||
| E_all_lo = 2.781 |
There was a problem hiding this comment.
could you add in-line comments about what each of these variables are and the units
E_all_lo = 2.781 # [kWh/kg] the something something of somethingThere was a problem hiding this comment.
Added as a comment block at the top so I didn't have to repeat 3 times, fine if you want to change
There was a problem hiding this comment.
Added as a comment block at the top so I didn't have to repeat 3 times, fine if you want to change
| n_timesteps = self.options["plant_config"]["plant"]["simulation"]["n_timesteps"] | ||
| self.config = HumbertEwinConfig.from_dict( | ||
| merge_shared_inputs(self.options["tech_config"]["model_inputs"], "performance"), | ||
| strict=False, |
There was a problem hiding this comment.
why strict=False - this is normally only used (as far I I know) for dispatch/storage models because theres more than 2 models connected. But in this case, it appears that its only a performance and cost model, so strict=True would be appropriate
There was a problem hiding this comment.
Not sure if I need to change to strict=True or remove the whole statement - will leave in your hands!
| self.add_input("spec_energy_electrolysis", val=E_electrolysis, units="kW*h/kg") | ||
| self.add_input("capacity", val=self.config.capacity_mw, units="MW") | ||
|
|
||
| self.add_output( |
There was a problem hiding this comment.
could you add iron_ore_consumed as an output here and then calculate it in the compute() method?
|
|
||
| # MSE - Opex | ||
| positions = 499.2 / 2e6 # Labor rate (position-years/tonne) | ||
| NaOH_ratio = 0 # Ratio of NaOH consumption to annual iron production |
There was a problem hiding this comment.
should NaOH and CaCl2 be inputs to the performance model that are consumed and may limit the output production similar to electricity and iron ore? Then the costs associated with these could be represented with a feedstock component and not included in this cost model. I could see it going either way, if it makes sense to include within this cost model, then its fine as-is
There was a problem hiding this comment.
Yes, it makes sense to make them into feedstocks and limit production. If you can make that change really quick great, I don't see it as a requirement - I see a wave of converting lots of these little VarOpExes into feedstocks at once across multiple models, because I bet there's more of them buried.
| anode_replace_int = 3 # Replacement interval of anodes (years) | ||
|
|
||
| # Set up connected inputs | ||
| self.add_input("output_capacity", val=0.0, units="Mg/year") # Mg = tonnes |
There was a problem hiding this comment.
why not use t/yr instead of Mg/yr?
| elec_in = inputs["electricity_in"] | ||
| elec_price = inputs["price_electricity"] | ||
|
|
||
| # Add ore transport cost TODO: turn iron_transport into proper transporter |
There was a problem hiding this comment.
Reminder to remove the transport cost from this model since its a separate component now.
There was a problem hiding this comment.
Already removed, also removing reference in docstring
|
|
||
| # Humbert Opex model - from SI spreadsheet (doi.org/10.1007/s40831-024-00878-3) | ||
| # Default costs - adjusted to 2018 to match Stinn via CPI | ||
| labor_rate = 55.90 # USD/person-hour |
There was a problem hiding this comment.
could these costs be in the configuration class? They could default to the values you have here but could then be modified by the user.
There was a problem hiding this comment.
Yes, those could easily be config parameters. Lazy on my part. If you have time to fix, please do
| outputs["VarOpEx"] = ( | ||
| NaOH_opex + CaCl2_opex + limestone_opex + anode_opex + ore_opex + elec_opex | ||
| ) | ||
| outputs["labor_opex"] = labor_opex |
There was a problem hiding this comment.
Are you outputting the individual costs for testing purposes to make sure it's aligned with the results from the paper? (Just checking since only OpEx, CapEx, and VarOpEx will be used in the finance models)
There was a problem hiding this comment.
I want to have the ability for capex and opex to be broken out in more detail - I don't want the only outputs of the cost model to be the entire sum of capex and opex. I want to be able to make plots and tables (straight from the output files) that show which parts of the capex (the electrolyzer, the processing) are sensitive to which inputs (the temperature, the iron content of the ore. Even if the finance model isn't using them I'd like to keep these outputs, and I'll put a note into the comments so there's no confusion on why they're there.
| self.add_input("positions", val=positions, units="year/Mg") | ||
| self.add_input("NaOH_ratio", val=NaOH_ratio, units=None) | ||
| self.add_input("CaCl2_ratio", val=CaCl2_ratio, units=None) | ||
| self.add_input("limestone_ratio", val=limestone_ratio, units=None) |
There was a problem hiding this comment.
instead of units of None could you make these unitless?
| self.add_input("iron_ore_in", val=0.0, shape=n_timesteps, units="kg/h") | ||
| self.add_input("iron_transport_cost", val=0.0, units="USD/t") | ||
| self.add_input("price_iron_ore", val=0.0, units="USD/Mg") | ||
| self.add_input("electricity_in", val=0.0, shape=n_timesteps, units="kW") |
There was a problem hiding this comment.
I think electricity and iron ore costs should be calculated in feedstock models
There was a problem hiding this comment.
Indeed! Looks like you already removed, also deleting references from docstrings
Iron: Adding electrowinning capabilities
This PR adds three iron electrowinning technologies:
For the time being, the performance and cost are modeled fairly simply. Performance is modeled based on upper and lower bounds of energy required for each type as defined by Humbert. Cost is modeled based on correlations of capital cost developed for electrowinning of many different metals developed by Stinn and Allanore, and an opex model developed by Humbert in the spreadsheet available as SI to the linked paper, which also provides input values for the Stinn/Allanore capex model.
Section 1: Type of Contribution
Section 2: Draft PR Checklist
N/A
Type of Reviewer Feedback Requested (on Draft PR)
Structural feedback:
Implementation feedback:
Other feedback:
Section 3: General PR Checklist
docs/files are up-to-date, or added when necessaryCHANGELOG.mdhas been updated to describe the changes made in this PRSection 3: Related Issues
Section 4: Impacted Areas of the Software
Section 4.1: New Files
h2integrate\converters\iron\:humbert_ewin_perf.py: New iron electrowinning performance modelhumbert_electrowinning_performancebased on Humberthumbert_stinn_ewin_cost.py: New iron electrowinning cost modelhumbert_stinn_electrowinning_costbased on Humbert and Stinn and Allanorestinn\table1.csv: Input data table mostly from Stinn and Allanore but iron costs from Humberth2integrate\converters\iron\stinn\cost_model.py: Modified the transcription of the Humbert SI equations to interface with thehumbert_stinn_electrowinning_costmodelexamples\27_iron_electrowinning\: New example that produces an LCOI for all 3 types of iron electrowinningSection 4.2: Modified Files
h2integrate\converters\iron\stinn\:cost_model.py: Modified the transcription of the Stinn and Allanore equations to interface with thehumbert_stinn_electrowinning_costmodelcost_coeffs.csv: Added in some coefficients that were previously hard-coded intocost_model.pyh2integrate\core\supported_models.py: Addedhumbert_electrowinning_performanceandhumbert_stinn_electrowinning_costas supported modelsh2integrate\finances\profast_base.py: Addedsponge_ironas a mass commoditySection 5: Additional Supporting Information
Section 6: Test Results, if applicable
Section 7 (Optional): New Model Checklist
docs/developer_guide/coding_guidelines.mdattrsclass to define theConfigto load in attributes for the modelBaseConfigorCostModelBaseConfiginitialize()method,setup()method,compute()methodCostModelBaseClasssupported_models.pycreate_financial_modelinh2integrate_model.pytest_all_examples.pydocs/user_guide/model_overview.mddocs/section<model_name>.mdis added to the_toc.yml