Update nf scaling. #1145

avdudchenko · 2023-09-22T05:27:33Z

Fixes/Resolves:

This is meant to improve nf stability when adding new ions, with low concentrations, and simplifies some of the scaling calculations.
(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)

Summary/Motivation:

When adding Sr and Ba to NF flowsheet for external analysis, it was found that dual infesability was 3e-3 ,while for the current NF flowsheet the dual infeasibility was 1e-7. This change fixes the estimates for ion flux scailing, which improves dual infesaiblity, both in external flowsheet (to 7e-6) and watertap flowsheet reducing dual infeasibility to 1e-8 and hopefully, improving the stability of the flowsheets. Further improvments will need to be made for low ion concentrations, where dual infeasibility seems to grow.

Report from ipopt before updated scailing for psudo-box problem solve:

Number of Iterations....: 14
                                    (scaled)                 (unscaled)
Objective...............:   0.0000000000000000e+00    0.0000000000000000e+00
Dual infeasibility......:   2.9103830456733704e-11    1.4126505637889256e-07
Constraint violation....:   8.5265128291212022e-14    4.8316906031686813e-13
Complementarity.........:   2.5059035596800626e-09    2.5059035596800626e-09
Overall NLP error.......:   7.0141214751885410e-10    1.4126505637889256e-07


Number of objective function evaluations             = 18
Number of objective gradient evaluations             = 15
Number of equality constraint evaluations            = 18
Number of inequality constraint evaluations          = 18
Number of equality constraint Jacobian evaluations   = 15
Number of inequality constraint Jacobian evaluations = 15
Number of Lagrangian Hessian evaluations             = 14
Total CPU secs in IPOPT (w/o function evaluations)   =      0.029
Total CPU secs in NLP function evaluations           =      0.000

Output with updated scaling:

Number of Iterations....: 15
                                   (scaled)                 (unscaled)
Objective...............:   0.0000000000000000e+00    0.0000000000000000e+00
Dual infeasibility......:   2.9103830456733704e-11    2.1460611066004273e-08
Constraint violation....:   3.6948222259525210e-13    3.6948222259525210e-13
Complementarity.........:   2.5059035596800626e-09    2.5059035596800626e-09
Overall NLP error.......:   7.0141196129320849e-10    2.1460611066004273e-08


Number of objective function evaluations             = 21
Number of objective gradient evaluations             = 16
Number of equality constraint evaluations            = 21
Number of inequality constraint evaluations          = 21
Number of equality constraint Jacobian evaluations   = 16
Number of inequality constraint Jacobian evaluations = 16
Number of Lagrangian Hessian evaluations             = 15
Total CPU secs in IPOPT (w/o function evaluations)   =      0.030
Total CPU secs in NLP function evaluations           =      0.001

Changes proposed in this PR:

-flux scaling is calculated based on water flux, feed ion concentration, and rejection estimated based on ion valance, and ion size relative to membrane pore size - this can use improvement in the future.
-some small updated to other calculated scailing factors (they produce equivalent scaling factors as before, but simplify the code)

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-09-22T07:07:07Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4b2c793) 94.88% compared to head (9df6623) 94.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1145      +/-   ##
==========================================
+ Coverage   94.88%   94.89%   +0.01%     
==========================================
  Files         341      341              
  Lines       34481    34549      +68     
==========================================
+ Hits        32716    32784      +68     
  Misses       1765     1765

Files	Coverage Δ
watertap/examples/flowsheets/nf_dspmde/nf.py	`93.17% <100.00%> (-0.07%)`	⬇️
...rtap/property_models/multicomp_aq_sol_prop_pack.py	`96.14% <ø> (ø)`
watertap/unit_models/nanofiltration_DSPMDE_0D.py	`92.49% <100.00%> (+0.90%)`	⬆️

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

adam-a-a · 2023-09-22T16:06:48Z

watertap/unit_models/nanofiltration_DSPMDE_0D.py

+                    rejection_factor = (
+                        10 ** abs(self.permeate_side[t, x].charge_comp[j].value) - 1
+                    ) / 10 ** abs(self.permeate_side[t, x].charge_comp[j].value)


I am trying to bring these into my PR and see what the result will be. However, is rejection_factor making much difference? The comment says will be 9, 99, 999 for 1,2,3 valence, but as written, it would be 0.9, 0.99, 0.999, leading me to believe that `rejection_factor might not be critical. Sanity check?

Ah- it should actually make a difference since you are doing (1-rejection_factor) below.

Yea. That was not the most clear note. should of added the %. We also need to think about how to deal with "physical hinderence".....wait. dont we have an expression for that in NF model already ....

lambda_comp is the ratio between stokes radius of solute and membrane pore radius, so we technically could've used that in place of your manual writing of it although it is an expression with a smooth_min function in it, so I didn't press for it. We do have convective and diffusive hindrance factors in the NF model if that's what you were hinting at.

Yeah, I actually tested out using diffus_pore_comp, but it made a minor change to unscaled tolerances. So I did not update it.

The issue still remains for me, that if I add low concentration of ion like Sr or Ba, my dual infeasibility is at about 7e-6, ideally, we want to push that to 1e-8, like in the current nf flowsheet. but there is still something missing in the puzzle. I wounder if we are focusing on the wrong scaling. One other thought came to mind, that we don't properly scale concentrations at membrane water interface, which would be higher than bulk...maybe that is worth looking at next for me.

I will keep this PR up so we can use it as a test bed, there must be a better way to do this scailing.

Sounds good. I will put this PR into draft mode to avoid confusion by other potential reviewers.

Great. Go ahead and push PR#1136 first. I want to keep this up so I can test linux/windows impacts, and propose new scailing approaches if there is somethhing I figure out that's cleaner/better.

adam-a-a · 2023-09-22T20:38:24Z

Trying to merge PR #1136 first and @avdudchenko wants to keep this up to try a few more mods after the fact.

…atertap into nf_scailing_update

adam-a-a · 2023-09-26T22:14:59Z

watertap/examples/flowsheets/nf_dspmde/nf.py

@@ -275,7 +275,6 @@ def unfix_opt_vars(m):
    m.fs.NF.nfUnit.velocity.unfix()
    m.fs.NF.nfUnit.velocity.setub(0.25)
    m.fs.product.max_hardness.fix(500)
-    m.fs.NF.nfUnit.recovery_vol_phase.fix(0.90)


Does this still break if you fix to 90% after scale refinement (considering all the latest changes with all checks passing)?

I still would prefer fixing recovery rate here simply for the sake of avoiding future test failures due to recov rate riding up to the upper bound and subtle variations that could occur from that, but going to approve this anyway and leave the choice to you

adam-a-a · 2023-09-26T22:19:29Z

watertap/examples/flowsheets/nf_dspmde/tests/test_nf.py

        "recovery": [
            m.fs.NF.nfUnit.recovery_vol_phase[0.0, "Liq"] * 100,
-            90,
+            94.64,


My only concern about letting recovery be "optimized" is that it'll drive up to the upper bound and at least previously, the test solution will deviate by a little every now and then with new changes in future PRs. It may be the case that the latest changes to scaling have remedied the issue completely, but it might also be the case that we'll see future tests fail because the solution converged to another recovery value close to 95% upper bound but outside of the tolerance of the regression test.

adam-a-a · 2023-09-26T22:22:35Z

watertap/unit_models/nanofiltration_DSPMDE_0D.py

-            iscale.set_scaling_factor(self.membrane_thickness_effective, 1e7)
+            iscale.set_scaling_factor(
+                self.membrane_thickness_effective,
+                1 / self.membrane_thickness_effective.value,


Generally, it's better to go with value(object) over object.value (after importing from pyomo.environ) since the current approach would work with variables but fail with expressions, whereas using value(object) should work across the board.

watertap/unit_models/nanofiltration_DSPMDE_0D.py

watertap/unit_models/tests/test_nanofiltration_DSPMDE_0D.py

adam-a-a

LGTM - just a couple of questions/comments before we merge this

watertap/unit_models/nanofiltration_DSPMDE_0D.py

adam-a-a · 2023-09-29T13:28:59Z

watertap/unit_models/nanofiltration_DSPMDE_0D.py

                    iscale.set_scaling_factor(v, sf)

+        for (t, p, j), v in self.flux_mol_phase_comp_avg.items():


This is an Expression and I never add scaling for them. Did this make a difference?

I see- you use it later on...thought it looks like you are just using the scaling factor of flow_mol_phase_comp

adam-a-a · 2023-09-29T14:12:13Z

watertap/unit_models/nanofiltration_DSPMDE_0D.py

@@ -1904,6 +1926,7 @@ def calculate_scaling_factors(self):

        # these variables do not typically require user input,
        # will not override if the user does provide the scaling factor
+        # make sure we scale solvent first!


Good catch!

watertap/unit_models/nanofiltration_DSPMDE_0D.py

adam-a-a · 2023-09-29T14:16:43Z

watertap/unit_models/nanofiltration_DSPMDE_0D.py

+            hfd = value(self.hindrance_factor_diffusive_comp[t, j])
+            bs = value(self.partition_factor_born_solvation_comp[t, j])
+            fdc = value(self.partition_factor_donnan_comp_feed[t, x, j])
+            hinderence_factor = hfd * bs * fdc


Suggested change

hinderence_factor = hfd * bs * fdc

hindrance_factor = hfd * bs * fdc

adam-a-a · 2023-09-29T14:43:32Z

watertap/unit_models/nanofiltration_DSPMDE_0D.py

+            if (
+                membrane_charge > 0
+                and value(self.permeate_side[t, x].charge_comp[j]) > 0
+            ):
+                charge_reflection_multiplier = 10 ** -abs(
+                    value(self.permeate_side[t, x].charge_comp[j])
+                )
+            elif (
+                membrane_charge < 0
+                and value(self.permeate_side[t, x].charge_comp[j]) < 0
+            ):
+                charge_reflection_multiplier = 10 ** -abs(
+                    value(self.permeate_side[t, x].charge_comp[j])
+                )
+            else:
+                charge_reflection_multiplier = 10 ** abs(
+                    value(self.permeate_side[t, x].charge_comp[j])
+                )
+            iscale.constraint_scaling_transform(
+                con, 1 / (hinderence_factor * charge_reflection_multiplier)
+            )


Same suggestion for condensing as above

adam-a-a · 2023-09-29T14:54:24Z

watertap/unit_models/nanofiltration_DSPMDE_0D.py

+                ** -1
+            ) ** -1


Hmm... this is all well and good, following that logic of equivalent resistance in parallel circuits, but do you have an extra power of -1 that should be removed here?

Its becouse we need to sum this with scailing factor for membrane charge which I do later down.
(e.g ((sum(x^-1)^-1)^-1+mem_charge^-1)^-1

Makes sense- we could then cancel these powers out, but won't functionally change anything. I'll leave the choice to you.

watertap/unit_models/nanofiltration_DSPMDE_0D.py

Co-authored-by: Adam Atia <aatia@keylogic.com>

This reverts commit d18652b97e5816d1dad853ba3f587675ca8b9ddd.

watertap/unit_models/nanofiltration_DSPMDE_0D.py

Co-authored-by: Adam Atia <aatia@keylogic.com>

adam-a-a

LGTM!

avdudchenko added 2 commits September 21, 2023 22:11

Update nanofiltration_DSPMDE_0D.py

14b0274

Update nf_with_bypass.py

e8d8df2

avdudchenko requested review from adam-a-a and bknueven September 22, 2023 05:27

avdudchenko added 2 commits September 21, 2023 23:12

Relaxed hardness constraint in jupyer notebook, and fixed test

6339842

Fixing black formating?

498cda2

bknueven approved these changes Sep 22, 2023

View reviewed changes

adam-a-a reviewed Sep 22, 2023

View reviewed changes

adam-a-a mentioned this pull request Sep 22, 2023

MCAS Update #1136

Merged

adam-a-a marked this pull request as draft September 22, 2023 20:40

avdudchenko added 9 commits September 22, 2023 17:39

Update nanofiltration_DSPMDE_0D.py

7dbe1ca

Merge branch 'main' into nf_scailing_update

1ea8928

update scaiing

9e2615f

Merge branch 'nf_scailing_update' of https://github.com/avdudchenko/w…

a8ec4c7

…atertap into nf_scailing_update

updated scailing/major patch

c8f8315

Update nanofiltration_DSPMDE_0D.py

50d5de4

fixed formating and removed prints

17e4d18

Update nanofiltration_DSPMDE_0D.py

54fefd1

Update nanofiltration_DSPMDE_0D.py

2c4d4aa

adam-a-a reviewed Sep 26, 2023

View reviewed changes

watertap/unit_models/nanofiltration_DSPMDE_0D.py Outdated Show resolved Hide resolved

Update watertap/unit_models/nanofiltration_DSPMDE_0D.py

740c772

adam-a-a reviewed Sep 26, 2023

View reviewed changes

watertap/unit_models/tests/test_nanofiltration_DSPMDE_0D.py Outdated Show resolved Hide resolved

adam-a-a and others added 3 commits September 26, 2023 18:29

Update watertap/unit_models/tests/test_nanofiltration_DSPMDE_0D.py

5e8f95d

Merge remote-tracking branch 'upstream/main' into nf_scailing_update

be85e4f

updarted partioning scailign

6cc035b

avdudchenko added 3 commits September 27, 2023 08:20

Update nanofiltration_DSPMDE_0D.py

1b599d1

Update nanofiltration_DSPMDE_0D.py

02f00db

Update nawi_spring_meeting2023.ipynb

281a992

adam-a-a requested a review from bknueven September 27, 2023 16:03

avdudchenko added 2 commits September 27, 2023 09:03

Update nawi_spring_meeting2023.ipynb

1644dfc

removed duplicate calc scailign

5ba4974

avdudchenko marked this pull request as ready for review September 27, 2023 19:05

ksbeattie added the Priority:Normal Normal Priority Issue or PR label Sep 28, 2023

ksbeattie assigned adam-a-a Sep 28, 2023

Merge branch 'main' into nf_scailing_update

6ea0e3e

adam-a-a reviewed Sep 29, 2023

View reviewed changes

avdudchenko and others added 9 commits September 29, 2023 14:18

Update watertap/unit_models/nanofiltration_DSPMDE_0D.py

ce6f865

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update watertap/unit_models/nanofiltration_DSPMDE_0D.py

7857c71

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update watertap/unit_models/nanofiltration_DSPMDE_0D.py

8771da9

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update watertap/unit_models/nanofiltration_DSPMDE_0D.py

18c5574

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update nanofiltration_DSPMDE_0D.py

4b6ebae

Update nanofiltration_DSPMDE_0D.py

f89e036

Revert "Update multicomp_aq_sol_prop_pack.py"

b1fea16

This reverts commit d18652b97e5816d1dad853ba3f587675ca8b9ddd.

Update nanofiltration_DSPMDE_0D.py

0e22f60

Update nawi_spring_meeting2023.ipynb

5918800

adam-a-a reviewed Sep 29, 2023

View reviewed changes

watertap/unit_models/nanofiltration_DSPMDE_0D.py Outdated Show resolved Hide resolved

avdudchenko and others added 3 commits September 29, 2023 16:35

Update watertap/unit_models/nanofiltration_DSPMDE_0D.py

3be712a

Co-authored-by: Adam Atia <aatia@keylogic.com>

Merge branch 'main' into nf_scailing_update

ee46fb6

Apply suggestions from code review

632d997

adam-a-a approved these changes Oct 2, 2023

View reviewed changes

adam-a-a changed the title ~~Update nf scailing.~~ Update nf scaling. Oct 2, 2023

pylint<3 (for now)

9df6623

adam-a-a mentioned this pull request Oct 3, 2023

Add GUI for RO flowsheet #1159

Merged

adam-a-a merged commit 5b582df into watertap-org:main Oct 3, 2023
24 checks passed

lbianchi-lbl mentioned this pull request Oct 3, 2023

Resolve Pylint 3.0 errors #1161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update nf scaling. #1145

Update nf scaling. #1145

avdudchenko commented Sep 22, 2023 •

edited by adam-a-a

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading

adam-a-a Sep 22, 2023

adam-a-a Sep 22, 2023

avdudchenko Sep 22, 2023

adam-a-a Sep 22, 2023

avdudchenko Sep 22, 2023

adam-a-a Sep 22, 2023 •

edited

Loading

avdudchenko Sep 22, 2023

adam-a-a commented Sep 22, 2023

adam-a-a Sep 26, 2023 •

edited

Loading

adam-a-a Oct 2, 2023

adam-a-a Sep 26, 2023

adam-a-a Sep 26, 2023

adam-a-a left a comment

adam-a-a Sep 29, 2023

adam-a-a Sep 29, 2023

adam-a-a Sep 29, 2023

adam-a-a Sep 29, 2023

adam-a-a Sep 29, 2023

avdudchenko Sep 29, 2023

adam-a-a Sep 29, 2023

avdudchenko Sep 29, 2023

adam-a-a Sep 29, 2023

adam-a-a left a comment

		iscale.set_scaling_factor(v, sf)

		for (t, p, j), v in self.flux_mol_phase_comp_avg.items():

	hinderence_factor = hfd * bs * fdc
	hindrance_factor = hfd * bs * fdc

Update nf scaling. #1145

Update nf scaling. #1145

Conversation

avdudchenko commented Sep 22, 2023 • edited by adam-a-a Loading

Fixes/Resolves:

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented Sep 22, 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

adam-a-a Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-a-a commented Sep 22, 2023

adam-a-a Sep 26, 2023 • 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

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

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

avdudchenko commented Sep 22, 2023 •

edited by adam-a-a

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading

adam-a-a Sep 22, 2023 •

edited

Loading

adam-a-a Sep 26, 2023 •

edited

Loading