ADM1/ASM2d Translator Update #1435

MarcusHolly · 2024-06-13T14:09:04Z

Summary/Motivation:

Correctly builds the translator block based on c-code from Flores-Alsina

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 · 2024-06-13T15:17:27Z

Codecov Report

Attention: Patch coverage is 99.34211% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.67%. Comparing base (a15e125) to head (d240381).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...ter_resource_recovery_facility/BSM2_P_extension.py	97.91%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1435   +/-   ##
=======================================
  Coverage   93.66%   93.67%           
=======================================
  Files         286      286           
  Lines       30697    30756   +59     
=======================================
+ Hits        28753    28810   +57     
- Misses       1944     1946    +2

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

adam-a-a

I have a few comments/questions on this.

adam-a-a · 2024-06-27T00:51:34Z

docs/technical_reference/unit_models/translators/translator_adm1_asm2d.rst

-
-
+   "Biomass concentration", ":math:`bio = X_{su, in} + X_{aa, in} + X_{fa, in} + X_{c4, in} + X_{pro, in} + X_{ac, in} + X_{h2, in} + X_{PAO, in}`"
+   "S_ac concentration", ":math:`S_{ac, 1} = S_{ac, in} + X_{PHA, in}`"


What are the 1 and 2 subscripts meant to represent (e.g., S_{ac, 1} or S_{ac, 2})?

Yes - the 1 and 2 subscripts correspond to the concentration of the given component at steps 1 and 2 (named like Sac_AD1 in the code). This translator only has 2 steps (whereas the other translator has 5).

A little bit confused here, e.g., I see you set X_{PP,1} = 0 and then there are some conversion equations like S_IP will add X_{PP,1}, but it is set as 0, so what is the purpose for this conversion? Add 0 does not seem to have any impacts of the outcome. and where does X_PP convert to?

I have done that just to be explicit, and the c-code also leaves in such terms even when they are set equal to 0. I wanted to avoid somebody cross-checking our code with the c-code and wondering why a term is missing from an expression

X_PP is converted into S_Mg, S_K, and S_IP

Yes, that is correct. It has no effect since it's zero. But is it possible that in the future we want to change X_PP from 0 to something else? If so, I think it's worth keeping this zero term here in the documentation and in the code itself. It's also just the most accurate representation of these equations.

@adam-a-a hope you could also join the talk, I am confused because I believe X_PP should be important components of the system, right now, it seems no matter the initial value of X_PP is, it will directly set to 0 and no actual conversion happens to the system. Same conditions to X_PHA and X_PAO

Oh... yea there's a mistake here. S_{IP, 1} should be X_{PP, in) + ... and not S_{IP, in) + ...

Great catch @luohezhiming. This may or may not screw up the flowsheet again, but I'll test it out now...

I will do a line-by-line review of the new translator file after this, which I just haven't had the time to do yet

...p/examples/flowsheets/case_studies/full_water_resource_recovery_facility/BSM2_P_extension.py

watertap/unit_models/translators/tests/test_translator_adm1_asm2d.py

watertap/unit_models/translators/translator_adm1_asm2d.py

…olly/watertap into new_adm1_asm2d_interface

adam-a-a

Still work to do, but at least we have this passing checks without costing!

adam-a-a · 2024-07-17T22:04:07Z

@luohezhiming let's merge this through once all checks pass and I will continue troubleshooting on PR #1467 while you work on modified ADM1 flowsheet

luohezhiming

The BSM2_electroNP.py needs to be deleted!

MarcusHolly added 3 commits June 13, 2024 00:20

Finish implementation of adm1/asm2d translator

af1dc9a

Resolve inconsistent unit errors

8318db2

Update test file

c0b8329

MarcusHolly added the Priority:Normal Normal Priority Issue or PR label Jun 13, 2024

MarcusHolly self-assigned this Jun 13, 2024

MarcusHolly requested a review from luohezhiming June 13, 2024 14:10

MarcusHolly added 15 commits June 13, 2024 11:35

Remove completed to-do item

b1273c4

Merge branch 'main' into new_adm1_asm2d_interface

df81af7

Add Soraya's scaling improvements

6b6a448

Address minor formatting issues

66a7c44

Add BSM2 flowsheet w/new translator

07ff096

New flowsheet solving with Bio_P=True

8133b7a

Minor improvements to BSM2

906fb13

BioP=True solves & BioP=False fails

4953299

Update translator documentation

f5ee78f

Correct imports in test file

49ce16c

Try automating scaling

5731370

Test autoscaling function

22c3091

Merge branch 'main' into new_adm1_asm2d_interface

98978a0

Update AD initialization

4159a92

Delete new files and update old files

22fd6a9

MarcusHolly requested a review from adam-a-a June 24, 2024 20:18

MarcusHolly added 3 commits June 26, 2024 16:30

Merge branch 'main' into new_adm1_asm2d_interface

321b16e

Add Chenyu's versions of solved flowsheets

a205a7a

Clean up flowsheets

87ccc2f

adam-a-a reviewed Jun 27, 2024

View reviewed changes

MarcusHolly added 3 commits June 26, 2024 23:29

Update test solutions

7150b66

Minor cleanup

b7b2c17

Add scaling factor that was accidentally deleted

d637ff4

MarcusHolly and others added 18 commits July 12, 2024 16:27

Put common translator parameters inside of ADM1 reaction package

c6700b5

Use watertap solver instead of IDAES

9bc5840

Update tests for bio_P=True and bio_P=False

b3cd2bf

Add config option for bio_P into GUI

95a6f9b

Address pylint issues

f8476c2

Try to resolve remaining test failures

cbbfe6f

Add watertap get_solver to test file

bb95f8d

Get rid of config option in BSM2-P GUI

cc3f243

Try to resolve GUI issue

b63ce95

Minor change to flowsheet scaling

4dc04b3

Add require_idaes_solver to BSM2-P GUI

4e8d9ed

Resolve solving issue

0729d39

Merge branch 'new_adm1_asm2d_interface' of https://github.com/MarcusH…

cc61553

…olly/watertap into new_adm1_asm2d_interface

Comment out everything related to costing

a7bdd21

Merge branch 'new_adm1_asm2d_interface' of https://github.com/MarcusH…

77bd768

…olly/watertap into new_adm1_asm2d_interface

Address Pylint issues

c06fe0f

Address Pylint issues

3313322

Merge branch 'main' into new_adm1_asm2d_interface

b18c7f2

adam-a-a approved these changes Jul 17, 2024

View reviewed changes

MarcusHolly requested a review from luohezhiming July 18, 2024 12:55

luohezhiming reviewed Jul 18, 2024

View reviewed changes

MarcusHolly unassigned luohezhiming Jul 18, 2024

Delete BSM2_electroNP flowsheet

53baefd

luohezhiming self-assigned this Jul 18, 2024

luohezhiming approved these changes Jul 18, 2024

View reviewed changes

Merge branch 'main' into new_adm1_asm2d_interface

88955db

adam-a-a enabled auto-merge (squash) July 18, 2024 18:28

adam-a-a requested a review from Morgan88888888 July 18, 2024 18:29

adam-a-a merged commit f94f21d into watertap-org:main Jul 18, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADM1/ASM2d Translator Update #1435

ADM1/ASM2d Translator Update #1435

MarcusHolly commented Jun 13, 2024

codecov bot commented Jun 13, 2024 •

edited

Loading

adam-a-a left a comment

adam-a-a Jun 27, 2024

MarcusHolly Jun 27, 2024 •

edited

Loading

luohezhiming Jul 2, 2024

MarcusHolly Jul 2, 2024

MarcusHolly Jul 2, 2024

MarcusHolly Jul 2, 2024

luohezhiming Jul 2, 2024

MarcusHolly Jul 2, 2024

MarcusHolly Jul 2, 2024

MarcusHolly Jul 2, 2024

adam-a-a left a comment

adam-a-a commented Jul 17, 2024

luohezhiming left a comment



		"Biomass concentration", ":math:`bio = X_{su, in} + X_{aa, in} + X_{fa, in} + X_{c4, in} + X_{pro, in} + X_{ac, in} + X_{h2, in} + X_{PAO, in}`"
		"S_ac concentration", ":math:`S_{ac, 1} = S_{ac, in} + X_{PHA, in}`"

ADM1/ASM2d Translator Update #1435

ADM1/ASM2d Translator Update #1435

Conversation

MarcusHolly commented Jun 13, 2024

Summary/Motivation:

Legal Acknowledgement

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

adam-a-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcusHolly Jun 27, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-a-a left a comment

Choose a reason for hiding this comment

adam-a-a commented Jul 17, 2024

luohezhiming left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 13, 2024 •

edited

Loading

MarcusHolly Jun 27, 2024 •

edited

Loading