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

Add documentation for electroNP, OARO and UV-AOP #1377

Merged
merged 105 commits into from
May 21, 2024

Conversation

luohezhiming
Copy link
Contributor

@luohezhiming luohezhiming commented May 6, 2024

Fixes/Resolves:

issue #1360

Summary/Motivation:

Changes proposed in this PR:

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.

Comment on lines 13 to 15
"Reactor sizing cost", ":math:`C_V`", "sizing_cost", "1000", ":math:`\text{$/m^3}`"
"Magnesium chloride cost", ":math:`C_{MgCl2}`", "magnesium_chloride_cost", "0.0786", ":math:`\text{$/kg}`"
"Phosphorus recovery value*", ":math:`C_{RP}`", "phosphorus_recovery_value", "-0.07", ":math:`\text{$/kg}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Units not displaying correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should decide on whether we want to just display $ or USD_YYYY as the units for cost. I've done the latter in my documentation so far, but whatever we decide to do, we should be consistent.


"description", ":math:`Symbol_{example}`", "variable_name", "1", ":math:`\text{dimensionless}`"
"Inlet volumetric flow rate", ":math:`Q_{in}`", "mixed_state[0].flow_vol", ":math:`\text{m^3/hr}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix superscript in m3

Comment on lines 12 to 14
"Membrane replacement factor", ":math:`f`", "factor_membrane_replacement", "0.15", ":math:`\text{dimensionless}`"
"Membrane cost", ":math:`C_A`", "membrane_cost", "30", ":math:`\text{$/m^2}`"
"High pressure membrane cost", ":math:`C_hA`", "high_pressure_membrane_cost", "50", ":math:`\text{$/m^2}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Units not displaying correctly


"description", ":math:`Symbol_{example}`", "variable_name", "1", ":math:`\text{dimensionless}`"
"Membrane area", ":math:`A`", "area", ":math:`\text{m^2}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix superscript

Comment on lines 13 to 14
"UV reactor cost", ":math:`C_F`", "reactor_cost", "202.346", ":math:`\text{$/(m^3/hr)}`"
"UV lamps, sleeves, ballasts and sensors cost", ":math:`C_l`", "lamp_cost", "235.5", ":math:`\text{$/kW}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Units not displaying correctly

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.92%. Comparing base (d0a0b4e) to head (88da188).
Report is 69 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1377   +/-   ##
=======================================
  Coverage   93.92%   93.92%           
=======================================
  Files         335      335           
  Lines       35620    35620           
=======================================
  Hits        33456    33456           
  Misses       2164     2164           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

luohezhiming and others added 3 commits May 6, 2024 15:03
Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>
Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

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

LGTM except for the small changes we discussed regarding the Costing Method Variables table.


.. csv-table::
:header: "Description", "Symbol", "Variable Name", "Default Value", "Units"
:header: "Description", "Symbol", "Variable Name", "Units"
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on feedback from yesterday, there should be a column for Index here instead of Default Value.

Comment on lines 20 to 22
:header: "Description", "Symbol", "Variable Name", "Default Value", "Units"
:header: "Description", "Symbol", "Variable Name", "Units"
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on feedback from yesterday, there should be a column for Index here instead of Default Value.

@ksbeattie ksbeattie added 1.0 Hard requirement for the 1.0 release Priority:High High Priority Issue or PR labels May 9, 2024
@luohezhiming luohezhiming requested a review from MarcusHolly May 9, 2024 20:38
Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

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

With the exception of the ElectroN-P and OARO "Costing Method Variables" table not being updated to include a column for index (as is done in the UV AOP file), everything LGTM.

Copy link
Contributor

@adam-a-a adam-a-a left a comment

Choose a reason for hiding this comment

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

Nice job!

@adam-a-a adam-a-a enabled auto-merge (squash) May 21, 2024 00:24
@adam-a-a adam-a-a merged commit 767910c into watertap-org:main May 21, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Hard requirement for the 1.0 release Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants