-
Notifications
You must be signed in to change notification settings - Fork 57
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
Fix ZO costing method documentation #1081
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1081 +/- ##
==========================================
- Coverage 95.53% 95.53% -0.01%
==========================================
Files 318 318
Lines 30991 30997 +6
==========================================
+ Hits 29608 29612 +4
- Misses 1383 1385 +2 ☔ View full report in Codecov by Sentry. |
docs/technical_reference/unit_models/zero_order_unit_models/automate_rst_file_wt3.py
Show resolved
Hide resolved
f.write("=" * (5 + len(u))) | ||
if __name__ == "__main__": | ||
|
||
# Create index file for all zero order model docs |
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.
From a quick skim, I am guessing that most of the modifications in this file had to do with (1) moving the bulk of the code under this conditional used for script execution and (2) revising conditionals for feed_zo since you added something to the unit model to account for lack of costing. Any other major changes that I missed (or misconstrued)?
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.
Besides the indentation, the only thing that has really changed is now the name and path of the costing method is pulled directly off the unit model class by checking its default_costing_method
attribute. See: https://github.com/bknueven/watertap/blob/79e85fea9bda3b969ed03a65a2677f01f26842c4/docs/technical_reference/unit_models/zero_order_unit_models/automate_rst_file_wt3.py#L382-L392
So in the Excel sheet now the costing_method
column really means "does this unit have costing?".
@@ -13,8 +13,8 @@ See documentation for :ref:`Helper Methods for Electricity Demand<electricity_me | |||
|
|||
Costing Method |
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.
Small thing, but I realized that "SMP" is not capitalized in the toc tree nor within the rst file. To change this, you can add an entry for smp in title_exceptions
in the automation script. I remember some talk about removing this unit all together, but since we have it here, might as well give it a name. @kurbansitterley do you remember what "SMP" stood for in WT3?
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.
Hm no I don't remember... soluble microbial products is the only SMP acronym I know.
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.
WT3 doesn't seem to know either.. https://github.com/search?q=repo%3ANREL%2FWaterTAP3%20SMP&type=code
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 elected to just capitalize SMP 3de7d0f
Fixes: #1064
Summary/Motivation:
See #1064.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: