ASM1/ADM1 Translator Documentation #1000

MarcusHolly · 2023-04-11T13:17:16Z

Fixes/Resolves:

Bullet point in master issue #934

Changes proposed in this PR:

Adds documentation for ASM1/ADM1 translator block
Minor corrections to ADM1/ASM1 translator block documentation
Minor corrections to doc strings in ASM1/ADM1 translator unit model file

Legal Acknowledgement

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

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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.

codecov · 2023-04-11T14:09:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.81%. Comparing base (7838694) to head (6c4ecde).
Report is 339 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1000   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files         294      294           
  Lines       28427    28427           
=======================================
  Hits        27237    27237           
  Misses       1190     1190

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

adam-a-a · 2023-04-11T22:08:29Z

docs/technical_reference/unit_models/translators/translator_asm1_adm1.rst

+
+   "Nitrogen fraction in particulate products", ":math:`i_{xe}`", "i_xe", 0.06, ":math:`\text{dimensionless}`"
+   "Nitrogen fraction in biomass", ":math:`i_{xb}`", "i_xb", 0.08, ":math:`\text{dimensionless}`"
+   "Anaerobic degradable fraction of X_I and X_P", ":math:`f_{xI}`", "f_xI", 0.05, ":math:`\text{dimensionless}`"


We should either (1) include links to the components tables for ASM1 and ADM1 for people to refer to follow this, or (2) the components tables for both ASM1 and ADM1 could be included directly to more easily follow the acronyms.

Option (2) would be ideal.

In fact, it would be good to do both: reference ASM1 and ADM1 property model documentation while also adding a single components section with tables for ASM1 and ADM1 components and descriptions (or two sections, i.e., ASM1 Components and ADM1 Components)

adam-a-a · 2023-04-11T22:12:57Z

docs/technical_reference/unit_models/translators/translator_asm1_adm1.rst

+S_nd and S_s Mapping Equations
+------------------------------
+
+.. figure:: ../../../_static/unit_models/translators/mapping_step_a.jpg


We need to reference any figures taken from the literature. Also, we should include the relevant reference(s) towards the end of the documentation from which the formulation of this translator block (Copp Interface if I'm not mistaken) originated from.

Oh, you did the latter!

adam-a-a · 2023-04-11T22:14:45Z

docs/technical_reference/unit_models/translators/translator_asm1_adm1.rst

+[1] Copp J. and Jeppsson, U., Rosen, C., 2006.
+Towards an ASM1 - ADM1 State Variable Interface for Plant-Wide Wastewater Treatment Modeling.
+Proceedings of the Water Environment Federation, 2003, pp 498-510.
+https://www.accesswater.org/publications/-290550/towards-an-asm1--ndash--adm1-state-variable-interface-for-plant-wide-wastewater-treatment-modeling


Link doesn't work for me

adam-a-a · 2023-04-11T22:16:17Z

docs/_static/unit_models/translators/mapping_step_a.jpg

Sorry to nitpick, but the resolution on the figures is a little low.

Let me take that back. I just went through readthedocs and it looks fine. When you click on the image to get a magnified view, it looks even better. One exception though for this figure: the dashed box was slightly cut off, so I would just replace with a snapshot of the full dashed box.

adam-a-a

Looks nice. I have a couple of requested changes.

yalinli2

Is there an example system using these translators? Would be cool if the user can run the example, can also help debugging.

But other than this and the one comment on S_co2, I don't have additional comments, thank you!

yalinli2 · 2023-04-12T14:33:06Z

docs/technical_reference/unit_models/translators/translator_asm1_adm1.rst

+  "Total anion equivalents concentration, S_an", ":math:`S_{an}`", "S_an"
+  "Carbon dioxide, S_co2", ":math:`S_{co2}`", "S_co2"
+
+**NOTE: S_h2 and S_ch4 have vapor phase and liquid phase, S_co2 only has vapor phase, and the other components only have liquid phase**


@adam-a-a I'm wondering where this comment is from? Do you have another component for dissolved CO2? If S_h2 and S_ch4 can have liquid phase, I'd expect S_co2 to have liquid phase as well as it's more dissoluble in the water

We do not have an explicit component for dissolved/liquid-phase CO2. Perhaps @adam-a-a or @agarciadiego can elaborate more on the rationale behind this comment.

I also asked the same question either on this or a previous PR, but based on the response I recall, the answer was our model only accounts for CO2 in gas phase on not the dissolved CO2 in liquid. I am not clear on how closely our model follows the literature on this specific aspect in terms of ASM1 model implementation, despite the fact that we know in reality there would be dissolved CO2. @agarciadiego - can you provide some insight since you worked on this part of ADM1?

MarcusHolly · 2023-04-12T15:16:26Z

@yalinli2 There are currently 2 use cases (that I know of) where this translator block is being used. The first is simply the test file for the unit model in isolation (https://github.com/watertap-org/watertap/blob/main/watertap/unit_models/translators/tests/test_translator_asm1_adm1.py) and the second is a flowsheet that consists of an ASM1/ADM1 translator block, an anaerobic digestor, and an ADM1/ASM1 translator block, but the PR is currently still a work in progress (#982)

adam-a-a · 2023-04-13T16:57:09Z

@yalinli2 There are currently 2 use cases (that I know of) where this translator block is being used. The first is simply the test file for the unit model in isolation (https://github.com/watertap-org/watertap/blob/main/watertap/unit_models/translators/tests/test_translator_asm1_adm1.py) and the second is a flowsheet that consists of an ASM1/ADM1 translator block, an anaerobic digestor, and an ADM1/ASM1 translator block, but the PR is currently still a work in progress (#982)

Just to add to that, that draft PR (#982) also includes the activated sludge process (with secondary clarifier modeled as simple separator, not Takacs model yet) before the ASM1/ADM1 translator block.

yalinli2 · 2023-04-13T17:24:47Z

@MarcusHolly @adam-a-a sounds great on the user case, #1002 on BSM2 would be a very useful user example/testing case

adam-a-a

LGTM- the only outstanding issue was referenced (#1004) for future discussion and could be resolved in a subsequent PR if needed.

Add translator documentation and minor doc string corrections

f41d2db

MarcusHolly added documentation Improvements or additions to documentation Priority:Normal Normal Priority Issue or PR labels Apr 11, 2023

MarcusHolly self-assigned this Apr 11, 2023

MarcusHolly added 2 commits April 11, 2023 09:42

Fix documentation failures

001a1e0

Try to fix image failure

337deb7

documentation refinement

28f0cc9

MarcusHolly marked this pull request as ready for review April 11, 2023 15:05

MarcusHolly requested review from adam-a-a, agarciadiego and luohezhiming April 11, 2023 15:06

MarcusHolly mentioned this pull request Apr 11, 2023

Implementing Benchmark Simulation Model No. 2 #934

Open

51 tasks

agarciadiego approved these changes Apr 11, 2023

View reviewed changes

adam-a-a reviewed Apr 11, 2023

View reviewed changes

adam-a-a requested changes Apr 11, 2023

View reviewed changes

adam-a-a requested a review from yalinli2 April 12, 2023 02:28

Address Adam's comments & carry over some changes to ADM1/ASM1

6c4ecde

MarcusHolly requested a review from adam-a-a April 12, 2023 12:40

yalinli2 reviewed Apr 12, 2023

View reviewed changes

adam-a-a mentioned this pull request Apr 13, 2023

Create how-to or tutorial example for BSM2 #1002

Closed

adam-a-a mentioned this pull request Apr 13, 2023

Should CO2 be in ADM1 liquid phase too? #1004

Closed

adam-a-a approved these changes Apr 13, 2023

View reviewed changes

adam-a-a merged commit 8a58894 into watertap-org:main Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASM1/ADM1 Translator Documentation #1000

ASM1/ADM1 Translator Documentation #1000

MarcusHolly commented Apr 11, 2023 •

edited

Loading

codecov bot commented Apr 11, 2023 •

edited

Loading

adam-a-a Apr 11, 2023

adam-a-a Apr 11, 2023

adam-a-a Apr 11, 2023

adam-a-a Apr 11, 2023

adam-a-a Apr 11, 2023

adam-a-a Apr 11, 2023

adam-a-a Apr 11, 2023 •

edited

Loading

adam-a-a left a comment

yalinli2 left a comment

yalinli2 Apr 12, 2023 •

edited

Loading

MarcusHolly Apr 12, 2023

adam-a-a Apr 13, 2023

MarcusHolly commented Apr 12, 2023 •

edited

Loading

adam-a-a commented Apr 13, 2023

yalinli2 commented Apr 13, 2023

adam-a-a left a comment

ASM1/ADM1 Translator Documentation #1000

ASM1/ADM1 Translator Documentation #1000

Conversation

MarcusHolly commented Apr 11, 2023 • edited Loading

Fixes/Resolves:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented Apr 11, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-a-a Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

adam-a-a left a comment

Choose a reason for hiding this comment

yalinli2 left a comment

Choose a reason for hiding this comment

yalinli2 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcusHolly commented Apr 12, 2023 • edited Loading

adam-a-a commented Apr 13, 2023

yalinli2 commented Apr 13, 2023

adam-a-a left a comment

Choose a reason for hiding this comment

MarcusHolly commented Apr 11, 2023 •

edited

Loading

codecov bot commented Apr 11, 2023 •

edited

Loading

adam-a-a Apr 11, 2023 •

edited

Loading

yalinli2 Apr 12, 2023 •

edited

Loading

MarcusHolly commented Apr 12, 2023 •

edited

Loading