-
Notifications
You must be signed in to change notification settings - Fork 24
Feature/bert basic #81
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
Conversation
| # stack_LCA = pd.concat([stack_LCA,temp_df]) | ||
| return { | ||
| "Refurb Schedule": sys_refurb, | ||
| "AEP [kWh/year]": sys_aep, |
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.
Is AEP here the input power to the electrolyzer 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.
I see now that it is, maybe we can call it power_used? I'm open to discussion on this!
genevievestarke
left a comment
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 great to me, just a few comments for clarity!
electrolyzer/simulation/bert.py
Outdated
| refturb_schedule, ahp_kg, aep_kWh = stack.estimate_life_performance_from_year( | ||
| plant_life_years | ||
| ) | ||
| sys_refurb = pd.concat( |
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 should be sys_AEP?
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.
good catch! thank you!
| self.DTSS = self.calc_state_space() | ||
| self.d_eol = self.calc_end_of_life_voltage() | ||
|
|
||
| def run_power_deg_penalty(self, P_in): |
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.
Is this function applying a penalty on power?
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.
yes - I added a doctoring!
| turndown_ratio: float | ||
| max_current_density: float | ||
|
|
||
| f_1: float |
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.
Can we define f_1 and f_2 here?
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.
The three cell model files (alkaline.py, cell.py, and pem.py) have a lot of commented out values. Can we clean those up so they're less confusing?
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 worked on cleaning up alkaline.py mostly. I have left some remaining comments because I think they would be useful for a) future development/bugfixes and b) documentation (once we get some doc pages more properly up n running). But - I did try to remove a lot of non-informative commented out code.
johnjasa
left a comment
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.
Thanks for your patience on this review, Elenya! I really like the amount of effort and organization you put into it. I think with a few relatively small changes, mostly for clarity and docstrings, this is good to come in. I've noted some places where I think you add more context, though I didn't individually mark every method that could use an updated docstring.
I also pushed up some extremely minor typographical updates directly to the branch.
| from electrolyzer.tools.type_dec import FromDictMixin | ||
|
|
||
|
|
||
| class BaseClass(FromDictMixin): |
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 this class is added in this PR but not used anywhere yet. Is that intended?
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.
yes. It will be updated in a following PR to function as a general base-class for the classes.
| # self.A_electrode = self.electrode["A_electrode"] | ||
| # self.e_a = self.electrode["e_a"] | ||
| # self.e_c = self.electrode["e_c"] |
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 is Gen's prior comment, but if these aren't used then feel free to remove them throughout
| # Ra = ( | ||
| # rho_nickle_eff | ||
| # * (self.e_a / self.A_electrode) | ||
| # * (1 + (temp_coeff * (T_C - tref))) | ||
| # ) |
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.
remove?
| # def calc_open_circuit_voltage(self, T_C): | ||
| # """ | ||
| # I [A]: current | ||
| # T_C [C]: temperature | ||
| # return :: E_rev0 [V/cell]: open-circuit voltage | ||
|
|
||
| # TODO: Are we correcting for temperature twice? U_rev0 should be just 1.229 and | ||
| # never change (possibly?) | ||
|
|
||
| # Reference: [Gambou, Guilbert,et al 2022]: Eqn 14 | ||
| # """ | ||
| # # General Nerst Equation | ||
| # # Eqn 14 of [Gambou, Guilbert,et al 2022] | ||
| # T_K = convert_temperature([T_C], "C", "K")[0] | ||
| # E_rev0 = ( | ||
| # 1.5184 | ||
| # - (1.5421 * (10 ** (-3)) * T_K) | ||
| # + (9.523 * (10 ** (-5)) * T_K * np.log(T_K)) | ||
| # + (9.84 * (10 ** (-8)) * (T_K**2)) | ||
| # ) | ||
| # # OR should this just be 1.229? | ||
| # # E_rev_fake = 1.229 |
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.
Remove?
| ) | ||
|
|
||
|
|
||
| F, _, _ = physical_constants["Faraday constant"] # Faraday's constant [C/mol] |
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 is fun! I haven't used scipy's constants much, nice idea.
electrolyzer/simulation/stack.py
Outdated
| """ | ||
| eol_eff_percent_loss [%]: efficiency drop that indicates end-of-life | ||
| (between 1 and 100) | ||
| """ |
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.
Update docstring to match what the method is doing
|
|
||
|
|
||
| @define | ||
| class Stack(FromDictMixin): |
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.
My overarching comment and suggestion for this Stack class is that there's a lot of pretty complex logic contained within its methods. I'd recommend giving a pretty detailed docstring for the class or methods that details what's going on. For instance, it took me a bit to trace through all the logic that happens when stacks are on or off and how time is tracked (maybe I'm a bit slow), but it'd be helpful for others to have a little guide for what's happening here.
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 tried to add some doc strings to some of the operation-based methods, although I am not 100% sure on how some of these functions work or are connected to the Supervisor class (I didn't make the controllers or these functions). Future development would have some of these "operation" type functions happening at the "cluster" level instead of the stack. This would also include updating the functionality for the system to work with feedback control (as it does now) or with open-loop operation (not an option at the moment).
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 wrote or helped with the operation functions so I can update those. Probably better to get this pull request in and put those in in a different pull request?
| stack_life = (1 / frac_of_life_used) * (self.uptime / 3600) # [hrs] | ||
| return stack_life | ||
|
|
||
| def estimate_life_performance_from_year(self, plant_life_years: int): |
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.
For most of these methods, a short docstring explaining what they're doing and what the logic is would be quite helpful
| hydrogen_degradation_penalty: | ||
| type: boolean | ||
| default: True | ||
| description: Toggle whether degradation is applied to hydrogen or power |
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.
If it's True, is the degradation applied to hydrogen? Maybe make that more clear in the description
| # A_electrode: | ||
| # type: number | ||
| # default: 300 | ||
| # description: electrode total area | ||
| # units: cm^2 | ||
| # e_a: | ||
| # type: number | ||
| # default: 0.2 | ||
| # description: anode thickness | ||
| # units: cm | ||
| # e_c: | ||
| # type: number | ||
| # default: 0.2 | ||
| # description: cathode thickness | ||
| # units: cm | ||
| e_e: |
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.
Remove if unused?
johnjasa
left a comment
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.
Thanks for the changes! This is a great step towards a better set of electrolyzer models in here.
genevievestarke
left a comment
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.
Overall, I think it looks great and we should merge in! I'd like to talk about how we apply the power degradation penalty at some point, but that doesn't have to be today.
| - **V_cell** (float): cell voltage in Volts | ||
| """ | ||
|
|
||
| I_stack = self.electrolyzer_model( |
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 still a bit confused how this applies a penalty on the power consumed. Maybe we can chat about it! Not a blocker, just to better understand the code.
* added updated degradation calcs * New degradation rates * updated test for new deg calcs * updat pre-commit yaml * remove whitespace * update test_regression * fix test_regression * fix test_regression * troubleshooting tests * test_regression total h2 4 decimals * Update test_run_electrolyzer.py * Added example #6, conversion efficiency * changed formatting for tests * reformatted for cleaned up deg rates * delete duplicate example * small formatting changes * WIP adjust modeling schema for alkaline cells * Bump isort version for pre-commit bugfix (#60) * alkaline example formatting * update test validation * update test_validation.py * updating test_run_electrolyzer with new modeling schema * fit_params difference * save for power_to_current debug * troubleshooting before test_run_lcoh.py * test_optimization new schema * tidying up a bit * controller example debugging * changed modeling schema interface in test_optimization * removing unnecessary comment blocks and unused files * updating examples for new schema * Issue #61: Update Cathod Pressure to be 30bar - added P_STD from scipy physical constants, which is 1 bar in Pascals - mods to calc_open_circuit_voltage() include updated p_cathode, split up partial pressures for hydrogen/oxygen according to Dalton's law. Issue #63: Nernst equation was incorrect. This issue only became became apparent when adjusting cathode pressure. - changed E_cell calculation to use the correct partial pressures and replaced R+T with R*T. - Updated test_cell.py, test_stack.py, test_run_electrolyzer.py to account for the new model change. - Incomplete is test_run_lcoh.py. Need assistance to adjust lcoh_breakdown. * Had to update LCOH test because of changes to cell voltage. Life Totals, RESULTS, and lcoh_results changed with latest cell modification. * add generic citation file * delete unneeded comments * resolve merge conflicts * Updated dryer loss to 0.2%. Test stack lowered the kWh/kg. Test run_electrolyzer produced more h2. * Test lcoh results in a lower lcoh. * Updated membrane thickness in calc_ohmic_overpot. 20 microns is closer to today's commercially available * Updated test stack and fit params. Needed to update example_01 by adding dt to stack_dict * Updated run_electrolyzer test. Hydrogen output increased * Updated run_lcoh test. Results show a lower lcoh * updating reg test value after changes from merge * changed PEM_Cell class to PEMCell * updating relative imports to explicit * updating Alkaline_Cell class to AlkalineCell * adding typing for stack.cell * fixing linting * removing commented code * WIP: Reformatting repo into BERT structure * updated all py file import paths * updated jupyter notebook import paths * updated pre-commit config * Prep for release (#82) * removed setup and added to pyproject * Updated CI workflow * WIP: Added CI and docs build * Added jupyterbook docs * Updated readme * change CI to install all * Update pyproject.toml * Feature/bert basic (#81) * reformatted * updated PEM schema and removed LCOH tests temporarily * updated alkaline modeling schema and alkaline Urev0 calc * update lcoh glue_code test regression values * added hydrogen loss from degradation and input toggle to choose that option * added LCA functions to stack.py * added eol_eff_percent_loss as degradation input for stack * added run_LCA functionto run_electrolyzer script * added more tracking variables to stack * updated all py file import paths * updated jupyter notebook import paths * added validators and minor fix in stack for cell type * added skeleton for cluster and cell classes * added generic base class * added example skeleton for simulation config * added file for power-curve function templates * updated inputs for Stack in test_stack * added import to fix tests to stack.py * removed empty PEM cell test * renamed test_cell to test_PEM_cell * updated validator format * removed files unrelated to primary feature-adds * reformatted bert.py * added back-in placeholder files * removed alkaline sources in alkaline model into md file * Typographical changes * minor cleanups to alkaline cell and stack * removed commented out code and added docstrings * fixed aep datadrame in bert.py * added more docstrings to stack.py * added comments to pem.py for faradaic coefficients --------- Co-authored-by: bayc <[email protected]> Co-authored-by: John Jasa <[email protected]> --------- Co-authored-by: ZackTully <[email protected]> Co-authored-by: Zachary Tully <[email protected]> Co-authored-by: nriccobo <[email protected]> Co-authored-by: elenya-grant <[email protected]> Co-authored-by: nRiccobo <[email protected]> Co-authored-by: bayc <[email protected]> Co-authored-by: Chris Bay <[email protected]> Co-authored-by: John Jasa <[email protected]>
* added updated degradation calcs * New degradation rates * updated test for new deg calcs * updat pre-commit yaml * remove whitespace * update test_regression * fix test_regression * fix test_regression * troubleshooting tests * test_regression total h2 4 decimals * Update test_run_electrolyzer.py * Added example #6, conversion efficiency * changed formatting for tests * reformatted for cleaned up deg rates * delete duplicate example * small formatting changes * WIP adjust modeling schema for alkaline cells * Bump isort version for pre-commit bugfix (#60) * alkaline example formatting * update test validation * update test_validation.py * updating test_run_electrolyzer with new modeling schema * fit_params difference * save for power_to_current debug * troubleshooting before test_run_lcoh.py * test_optimization new schema * tidying up a bit * controller example debugging * changed modeling schema interface in test_optimization * removing unnecessary comment blocks and unused files * updating examples for new schema * Issue #61: Update Cathod Pressure to be 30bar - added P_STD from scipy physical constants, which is 1 bar in Pascals - mods to calc_open_circuit_voltage() include updated p_cathode, split up partial pressures for hydrogen/oxygen according to Dalton's law. Issue #63: Nernst equation was incorrect. This issue only became became apparent when adjusting cathode pressure. - changed E_cell calculation to use the correct partial pressures and replaced R+T with R*T. - Updated test_cell.py, test_stack.py, test_run_electrolyzer.py to account for the new model change. - Incomplete is test_run_lcoh.py. Need assistance to adjust lcoh_breakdown. * Had to update LCOH test because of changes to cell voltage. Life Totals, RESULTS, and lcoh_results changed with latest cell modification. * add generic citation file * delete unneeded comments * resolve merge conflicts * Updated dryer loss to 0.2%. Test stack lowered the kWh/kg. Test run_electrolyzer produced more h2. * Test lcoh results in a lower lcoh. * Updated membrane thickness in calc_ohmic_overpot. 20 microns is closer to today's commercially available * Updated test stack and fit params. Needed to update example_01 by adding dt to stack_dict * Updated run_electrolyzer test. Hydrogen output increased * Updated run_lcoh test. Results show a lower lcoh * updating reg test value after changes from merge * changed PEM_Cell class to PEMCell * updating relative imports to explicit * updating Alkaline_Cell class to AlkalineCell * adding typing for stack.cell * fixing linting * removing commented code * WIP: Reformatting repo into BERT structure * updated all py file import paths * updated jupyter notebook import paths * updated pre-commit config * Prep for release (#82) * removed setup and added to pyproject * Updated CI workflow * WIP: Added CI and docs build * Added jupyterbook docs * Updated readme * change CI to install all * Update pyproject.toml * Feature/bert basic (#81) * reformatted * updated PEM schema and removed LCOH tests temporarily * updated alkaline modeling schema and alkaline Urev0 calc * update lcoh glue_code test regression values * added hydrogen loss from degradation and input toggle to choose that option * added LCA functions to stack.py * added eol_eff_percent_loss as degradation input for stack * added run_LCA functionto run_electrolyzer script * added more tracking variables to stack * updated all py file import paths * updated jupyter notebook import paths * added validators and minor fix in stack for cell type * added skeleton for cluster and cell classes * added generic base class * added example skeleton for simulation config * added file for power-curve function templates * updated inputs for Stack in test_stack * added import to fix tests to stack.py * removed empty PEM cell test * renamed test_cell to test_PEM_cell * updated validator format * removed files unrelated to primary feature-adds * reformatted bert.py * added back-in placeholder files * removed alkaline sources in alkaline model into md file * Typographical changes * minor cleanups to alkaline cell and stack * removed commented out code and added docstrings * fixed aep datadrame in bert.py * added more docstrings to stack.py * added comments to pem.py for faradaic coefficients --------- Co-authored-by: bayc <[email protected]> Co-authored-by: John Jasa <[email protected]> * Adding readme --------- Co-authored-by: ZackTully <[email protected]> Co-authored-by: Zachary Tully <[email protected]> Co-authored-by: nriccobo <[email protected]> Co-authored-by: elenya-grant <[email protected]> Co-authored-by: nRiccobo <[email protected]> Co-authored-by: Cameron Irmas <[email protected]> Co-authored-by: bayc <[email protected]> Co-authored-by: Chris Bay <[email protected]>
Feature-add: electrolyzer model partial sync-up
Primary changes
Related issue, if one exists
Impacted areas of the software
Modified files
electrolyzer/simulation/cell.pychanged toelectrolyzer/simulation/cell_models/pem.pytests/test_cell.pychanged totests/test_PEM_cell.pyelectrolyzer/simulation/bert.py: added run_LCA function which estimates lifetime performanceelectrolyzer/simulation/base.py: new file, added base classelectrolyzer/simulation/cell_models/cell.py: new file (empty right now) to function as cell model base classelectrolyzer/simulation/cell_models/alkaline.py: clean-ups and parameter renaming. Also fixed U_rev calc (used to be redundant)electrolyzer/simulation/cell_models/pem.py: addedp_anode,p_cathode,i_0_c,i_0_a,alpha_a,alpha_c,e_m,R_ohmic_elec,f_1, andf_2as attributes (replacing hard-coded values). Removeddryer_lossinput fromcalc_mass_flow_rate()electrolyzer/simulation/cluster.py: new file (empty right now) to contain cluster classelectrolyzer/simulation/stack.py: added end-of-life definition attributes (eol_eff_percent_loss) and variable that can be used to either increase power consumption or reduce hydrogen production as stack is degraded (hydrogen_degradation_penalty). Added two functions,run_power_deg_penalty()andrun_h2_deg_penalty()which are now called inrun()to apply degradation impacts to the power consumption or the hydrogen production. Also added new functions for calculating and estimating degradation impacts on lifetime performance which are:calc_end_of_life_voltage()estimate_time_until_replacement()estimate_stack_life()estimate_life_performance_from_yearelectrolyzer/tools/modeling_schema.yaml: updated for new inputs to stack and cell modelsTests changed:
tests/glue_code/test_run_electrolyzer.py: value changed because removed dryer losstests/glue_code/test_run_lcoh.py: values changed because removed dryer losstests/test_stack.py: added new input parameters to input dictionarytests/test_PEM_cell.py: added new input parameters to input dictionaryNew files:
docs/alkaline_ref.md: references for alkaline electrolyzer modelelectrolyzer/tools/validators.py: validatorsAdditional supporting information
Test results, if applicable
Tests pass
related issue