-
Notifications
You must be signed in to change notification settings - Fork 65
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
Electrolyzer costing documentation into template dedicated file #1392
Electrolyzer costing documentation into template dedicated file #1392
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.
LGTM - one small suggestion
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.
LGTM. Only needs a minor format fix.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1392 +/- ##
===========================================
+ Coverage 41.15% 93.92% +52.76%
===========================================
Files 97 335 +238
Lines 10572 35620 +25048
===========================================
+ Hits 4351 33456 +29105
+ Misses 6221 2164 -4057 ☔ View full report in Codecov by Sentry. |
"Total unit capital cost", ":math:`C_{cap}`", "capital_cost", "10,000", ":math:`\text{USD}_{2020}`" | ||
"Fixed operating costs", ":math:`C_{op}`", "fixed_operating_cost", "10,000", ":math:`\text{USD}_{2020}\text{/yr}`" | ||
"Membrane capital cost", ":math:`C_{cap,\, mem}`", "membrane_cost", "10,000", ":math:`\text{USD}_{2020}`" | ||
"Anode capital cost", ":math:`C_{cap,\, anode}`", "anode_cost", "10,000", ":math:`\text{USD}_{2020}`" | ||
"Cathode capital cost", ":math:`C_{cap,\, cathode}`", "cathode_cost", "10,000", ":math:`\text{USD}_{2020}`" | ||
"Membrane replacement cost", ":math:`C_{op,\, mem}`", "membrane_replacement_cost", "10,000", ":math:`\text{USD}_{2020}\text{/yr}`" |
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.
Forgot to mention this in my review, but we were planning on getting rid of the default value column in this table and replacing it with "Index" since a lot of these variables didn't have default values (as indicated by your placeholders) Adam will put up a PR that updates this for the auto-generated files, but it won't touch documentation that has already been updated. So we should update this before merging.
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.
@MarcusHolly I agree that default doesn't make sense here because it is basically the values that are initialized. However, I also don't know if any of these variables are indexed across any of the unit models, although in theory they could be. I went ahead and edited the Electrolyzer and GAC in this to lessen the number of PRs.
Fixes/Resolves:
Electrolyzer costing documentation for #1360
Mostly lateral changes from unit model documentation to costing model documentation files with minor wording changes
Summary/Motivation:
Cost documentation for all unit models
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: