Skip to content
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

Merged
merged 5 commits into from
Jul 13, 2023
Merged

Conversation

bknueven
Copy link
Contributor

Fixes: #1064

Summary/Motivation:

See #1064.

Changes proposed in this PR:

  • Update ZO rst file creator to dynamically write the location of ZO unit costing methods and re-factor of the rst file creator so its functions can be called in isolation (79e85fe)
  • Update resulting rst files (8172548)
  • Drive-by bug fix for costing, or lack thereof, on FeedZO and GasSpargedMembraneZO (888f363)

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@bknueven bknueven requested a review from adam-a-a July 10, 2023 22:23
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.53%. Comparing base (98aa405) to head (661de05).
Report is 271 commits behind head on main.

Files Patch % Lines
watertap/unit_models/zero_order/feed_zo.py 66.66% 1 Missing ⚠️
.../unit_models/zero_order/gas_sparged_membrane_zo.py 66.66% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

f.write("=" * (5 + len(u)))
if __name__ == "__main__":

# Create index file for all zero order model docs
Copy link
Contributor

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)?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Jul 13, 2023
@ksbeattie ksbeattie enabled auto-merge (squash) July 13, 2023 20:47
@ksbeattie ksbeattie merged commit d81564b into watertap-org:main Jul 13, 2023
23 checks passed
@bknueven bknueven deleted the issue_1064 branch September 22, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ZO model documentation wrt cost methods
5 participants