Replacing Expr_if & Changing Bio_P Default #1362

MarcusHolly · 2024-04-19T15:00:42Z

Fixes/Resolves:

Issue #934

Replaces the Expr_ifs in the ASM2d/ADM1 translator block (with the exception of 1 set of Expr_ifs) and changes the bio_P default to False such that the default setting does not set P components to 0 in the translator block.

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.

MarcusHolly · 2024-04-19T15:50:07Z

watertap/unit_models/translators/translator_asm2d_adm1.py

+            # TODO: Can this be replaced with smooth_max or smooth_min?
            @self.Expression(self.flowsheet().time, doc="Protein mapping")
            def Xpr_mapping(blk, t):


I'm not sure if this (and line 1039) can be expressed as a smooth_max or smooth_min

luohezhiming · 2024-04-22T20:32:09Z

watertap/unit_models/translators/translator_asm2d_adm1.py

                blk.properties_in[t].conc_mass_comp["S_F"] - blk.SN_org[t],
+                blk.eps,
            )


I am a little confused here, I think the original equation, if A>=B, then we have eps, but in the the smooth_max function, you directly change this eps to 0 if I understand correctly, and the eps in the smooth_max function is smoothing parameter set at 1e-10. My first question is: does change previous eps to 0 have no impacts to all of the examples? And why do you set 1e-10 for the smoothing parameter

Setting eps to 0 means it is no longer smooth, and can thus trigger numerical issues.

So there are two instances of eps in this file. eps is 0 and blk.eps is 1e-10. blk.eps is the one used in the smooth_max and smooth_min functions. I think I should just directly replace instances of eps with zero to make this less confusing

@luohezhiming I changed the epsilon used for smooth_max and smooth_min to eps_smooth to make things a bit more transparent. Also, the 1e-10 is the smoothing factor used in the ASM1/ADM1 translator, so I just carried that value over. It can be changed if necessary.

@MarcusHolly You should not be using eps=0 in the smoothmax and smoothmin functions - that defeats the entire purpose if those functions. If you really want a non-smooth max or min then you should refactor these to use Expr_if.

eps=1e-10 in the smooth_max. Are you saying it shouldn't be that close to 0 @andrewlee94?

Probably not - the value you use for eps represents how smooth the operation is. Smaller values mean less smoothness and thus more chance for numerical issues (but less numerical error). Ultimately, eps needs to be tuned for the application, and should be a few orders of magnitude less than the inputs to the max operation.

Changed the eps_smooth value to 1e-4 since this seems to give the best solver performance in the BSM2_P_extension.py flowsheet.

So the eps now only represents the smoothing parameter, we can try to vary the smooth_eps if all tests get passed.

…ewhere

MarcusHolly · 2024-04-23T15:15:45Z

When eps_smooth is 1e-10, the BSM2_P_extension test fails locally but passes the check here; however, when eps_smooth is 1e-4, the BSM2_P_extension test passes locally but fails the check here. Any thoughts @andrewlee94 or @adam-a-a ?

andrewlee94 · 2024-04-23T15:19:02Z

@MarcusHolly That is indicative of poor scaling. What do the diagnostics tools tell you?

MarcusHolly · 2024-04-23T15:33:29Z

@MarcusHolly That is indicative of poor scaling. What do the diagnostics tools tell you?

The scaling is definitely poor and/or non-existent in some places - this is a known issue. The DiagnosticToolbox identified 155 potential evaluation errors and variables/constraints with extreme jacobians, but none of these are new issues and are also present in the version currently merged in main.

codecov · 2024-04-26T21:31:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.85%. Comparing base (77d56e6) to head (0cc70ea).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1362   +/-   ##
=======================================
  Coverage   93.84%   93.85%           
=======================================
  Files         339      339           
  Lines       35887    35928   +41     
=======================================
+ Hits        33679    33720   +41     
  Misses       2208     2208

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

.../flowsheets/case_studies/full_water_resource_recovery_facility/test/test_BSM2_P_extension.py

MarcusHolly · 2024-05-03T19:00:40Z

@luohezhiming I've added a config option into the flowsheet so that the user can specify whether to use bio_P=True or bio_P=False.

luohezhiming · 2024-05-03T21:03:59Z

@luohezhiming I've added a config option into the flowsheet so that the user can specify whether to use bio_P=True or bio_P=False.

Looks good, Let's see if tests will get pass

luohezhiming

LGTM! Does all tests will pass with (using the new tear_guesses) bioP=True?

agarciadiego

LGTM

adam-a-a

My general question is how do the changes affect flowsheet convergence? I'd be interested to know, e.g., the number of iterations to solve before and after the changes, notable differences in IPOPT output, or anything of that nature. It would just be informative for all involved I think.

adam-a-a · 2024-05-10T00:33:04Z

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

-    }
+    if bio_P:
+        # Initial guesses for flow into first reactor
+        tear_guesses = {


How did you arrive at these new guesses?

I got the initial guesses from Chenyu's ElectroN-P flowsheet, and then I made some very slight modifications to them.

MarcusHolly · 2024-05-10T13:11:12Z

My general question is how do the changes affect flowsheet convergence? I'd be interested to know, e.g., the number of iterations to solve before and after the changes, notable differences in IPOPT output, or anything of that nature. It would just be informative for all involved I think.

Before changes (Bio_P = True):

After changes (Bio_P = True):

After changes (Bio_P = False):

andrewlee94 · 2024-05-10T13:25:31Z

That is a lot of iterations for solving a square problem - if IPOPT needs more than 50 iterations (and in practice often far fewer) on a square problem it often indicates the model has issues that should be resolved.

adam-a-a · 2024-05-10T14:09:51Z

That is a lot of iterations for solving a square problem - if IPOPT needs more than 50 iterations (and in practice often far fewer) on a square problem it often indicates the model has issues that should be resolved.

While we know that this model is problematic, we are continuously refining it as we go. We also suspect that poor scaling is significantly hampering the model. This flowsheet would actually be a great (and likely challenging) test case for the new scaling tools underway. In any case, we're making incremental improvements with every PR (or at least trying to).

MarcusHolly added 2 commits April 19, 2024 08:21

Import smooth max into translator block

6dec5c3

Replace Expr_ifs with smooth_max or smooth_min

46049a0

MarcusHolly added the Priority:Normal Normal Priority Issue or PR label Apr 19, 2024

MarcusHolly self-assigned this Apr 19, 2024

MarcusHolly mentioned this pull request Apr 19, 2024

Implementing Benchmark Simulation Model No. 2 #934

Open

51 tasks

Merge branch 'main' into replace_expr_ifs

ce7dccb

MarcusHolly marked this pull request as ready for review April 19, 2024 15:49

MarcusHolly requested review from adam-a-a, agarciadiego and luohezhiming April 19, 2024 15:49

MarcusHolly commented Apr 19, 2024

View reviewed changes

luohezhiming reviewed Apr 22, 2024

View reviewed changes

MarcusHolly added 2 commits April 23, 2024 08:23

differentiate between the eps smoothing function and the eps used els…

74b7ff6

…ewhere

Adjust the value of eps_smooth from 1e-10 to 1e-4

e9e442c

MarcusHolly marked this pull request as draft April 23, 2024 15:13

MarcusHolly added 9 commits April 24, 2024 13:12

Temporarily add diagnostic toolbox and scaling checks

73ff9b4

Merge branch 'main' into replace_expr_ifs

fdab851

Continue playing around with scaling and tear guesses

0956b3a

Reset flowsheet to the version in main

040e57e

Reset flowsheet to the version in main

3f574bb

Merge branch 'main' into replace_expr_ifs

05e53ec

Change bio_P default to False

a94da70

Replace separators with clarifiers and modify tear guesses

0880862

Update test

608c866

MarcusHolly marked this pull request as ready for review April 27, 2024 00:27

MarcusHolly changed the title ~~Replacing Expr_if in ASM2d/ADM1 Translator~~ Replacing Expr_if & Changing Bio_P Default Apr 27, 2024

MarcusHolly commented May 2, 2024

View reviewed changes

.../flowsheets/case_studies/full_water_resource_recovery_facility/test/test_BSM2_P_extension.py Outdated Show resolved Hide resolved

MarcusHolly marked this pull request as draft May 2, 2024 21:07

MarcusHolly added 2 commits May 3, 2024 14:51

Add bio_P config option to Extended BSM2 flowsheet and testing

a470c78

Address merge conflicts

f3c9e1c

MarcusHolly marked this pull request as ready for review May 3, 2024 18:59

Minor adjustments to test solutions

184fedf

MarcusHolly added 2 commits May 6, 2024 08:59

Minor improvements to bio_P=True tear guesses

91235fa

Clean up code

0cc70ea

MarcusHolly requested a review from luohezhiming May 6, 2024 16:37

luohezhiming approved these changes May 7, 2024

View reviewed changes

agarciadiego approved these changes May 9, 2024

View reviewed changes

adam-a-a reviewed May 10, 2024

View reviewed changes

adam-a-a approved these changes May 10, 2024

View reviewed changes

Merge branch 'main' into replace_expr_ifs

def8473

lbianchi-lbl enabled auto-merge (squash) May 10, 2024 14:22

lbianchi-lbl merged commit ac04ca7 into watertap-org:main May 10, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing Expr_if & Changing Bio_P Default #1362

Replacing Expr_if & Changing Bio_P Default #1362

MarcusHolly commented Apr 19, 2024 •

edited

Loading

MarcusHolly Apr 19, 2024

luohezhiming Apr 22, 2024 •

edited

Loading

andrewlee94 Apr 22, 2024

MarcusHolly Apr 22, 2024

MarcusHolly Apr 23, 2024

andrewlee94 Apr 23, 2024

MarcusHolly Apr 23, 2024

andrewlee94 Apr 23, 2024

MarcusHolly Apr 23, 2024 •

edited

Loading

luohezhiming Apr 23, 2024

MarcusHolly commented Apr 23, 2024

andrewlee94 commented Apr 23, 2024

MarcusHolly commented Apr 23, 2024

codecov bot commented Apr 26, 2024 •

edited

Loading

MarcusHolly commented May 3, 2024 •

edited

Loading

luohezhiming commented May 3, 2024

luohezhiming left a comment

agarciadiego left a comment

adam-a-a left a comment

adam-a-a May 10, 2024

MarcusHolly May 10, 2024

MarcusHolly commented May 10, 2024

andrewlee94 commented May 10, 2024

adam-a-a commented May 10, 2024

Replacing Expr_if & Changing Bio_P Default #1362

Replacing Expr_if & Changing Bio_P Default #1362

Conversation

MarcusHolly commented Apr 19, 2024 • edited Loading

Fixes/Resolves:

Legal Acknowledgement

Choose a reason for hiding this comment

luohezhiming Apr 22, 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

MarcusHolly Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcusHolly commented Apr 23, 2024

andrewlee94 commented Apr 23, 2024

MarcusHolly commented Apr 23, 2024

codecov bot commented Apr 26, 2024 • edited Loading

Codecov Report

MarcusHolly commented May 3, 2024 • edited Loading

luohezhiming commented May 3, 2024

luohezhiming left a comment

Choose a reason for hiding this comment

agarciadiego left a comment

Choose a reason for hiding this comment

adam-a-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcusHolly commented May 10, 2024

andrewlee94 commented May 10, 2024

adam-a-a commented May 10, 2024

MarcusHolly commented Apr 19, 2024 •

edited

Loading

luohezhiming Apr 22, 2024 •

edited

Loading

MarcusHolly Apr 23, 2024 •

edited

Loading

codecov bot commented Apr 26, 2024 •

edited

Loading

MarcusHolly commented May 3, 2024 •

edited

Loading