Update IDAES requirement to 2.2.0.dev0.watertap.23.08.03 and other stability improvements #930

bknueven · 2023-02-17T18:58:25Z

Fixes #929, Fixes #1072

Changes proposed in this PR:

Reverts f062086 (Make WaterTAP compatible with NL_v2 writer #929)
Updates IDAES tag to 23.08.03 (@lbianchi-lbl)
Changes to chemistry models for stability and to reflect upstream changes (Reformulate solubility_products constraints IDAES/idaes-pse#1197) (@lxhowl)
Fixes initialization / testing issue with NF w/ bypass flowsheet and notebook (Fix OS-specific solver issues for NF flowsheet #1072) (@lxhowl)
Remove call to clone from watertap/unit_models/test/test_gac.py for Pyomo 6.6.2 (@lxhowl)
Better scaling in IX0D test (@lxhowl)
Patch doctest for scaling factor warnings from NLv2 (@lxhowl)

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.

This reverts commit f062086.

…s, passed log_k_eq_constraint equilibrium_constraint for both solubility product and common eqbm mole_frac_phase_comp

… temprature Enthalpy unit corrected

This reverts commit f062086.

…s, passed log_k_eq_constraint equilibrium_constraint for both solubility product and common eqbm mole_frac_phase_comp

… temprature Enthalpy unit corrected

…ed solubility_product

… down as an additional variable Use smooth_max from idaes utility and other minor improvements

TimBartholomew

LGTM

adam-a-a · 2023-08-18T03:58:33Z

OK- I checked whether this PR would break anything in the BSM2 flowsheet by putting up a draft PR (#1108), and all checks pass! I'll review this and suspect it'll be ready to merge soon.

adam-a-a · 2023-08-18T04:02:23Z

docs/how_to_guides/how_to_use_a_property_model.rst

+   WARNING: model contains export suffix 'fs.state_block[0].scaling_factor' that
+   contains 4 component keys that are not exported as part of the NL file.
+   Skipping.
+   WARNING: model contains export suffix 'fs.state_block[0].scaling_factor' that
+   contains 4 component keys that are not exported as part of the NL file.
+   Skipping.


This looks like something we wouldn't want showing up in the how-to

It's just in the test -- so the reader won't see it.

I talked with @andrewlee94 about this a few weeks ago and there is not a workaround nor is one planned (IDAES and Pyomo are implementing a new model scaling framework)

adam-a-a · 2023-08-18T04:04:45Z

.github/workflows/checks.yml

@@ -304,4 +300,4 @@ jobs:
        pyomo build-extensions || python -c "from pyomo.contrib.pynumero.asl import AmplInterface; exit(0) if AmplInterface.available() else exit(1)"
    - name: Run pytest
      run: |
-        pytest --pyargs watertap -k 'not nf_dspmde.nf_ui'
+        pytest --pyargs watertap -k 'not (nf_dspmde.nf_ui or nf_dspmde.nf_with_bypass_ui)'


I suppose the alternative GUI flowsheet code was also causing checks to fail, hence it's addition here. DOes that mean the UI code for these NF flowsheets still has issues to address?

IIRC this is due to a limitation of the watertap.ui.fsapi layer for models that require the "good" (ma27-enabled) Ipopt to solve. If the model can be initialized without failures, specifying requires_idaes_solver=True kwarg is enough to skip the corresponding test without failures, but this doesn't work when the solve fails during initialization. Therefore, we have to exclude the entire test module using the -k not ... pytest flag.

Note -- this only applies to the macOS tests

watertap/examples/chemistry/tests/test_pure_water_pH.py

delete commented debugging code

adam-a-a · 2023-08-18T04:13:06Z

watertap/examples/chemistry/tests/test_pure_water_pH.py

@@ -585,7 +619,8 @@ def test_initialize(self, model_initialize):

    @pytest.fixture
    def model_solve(self, model, solver):
-        results = solver.solve(model, tee=True)
+        model.write("bah.nl", io_options={"symbolic_solver_labels": True})


Why is this here?

watertap/examples/chemistry/tests/test_solids.py

adam-a-a · 2023-08-18T04:17:59Z

...s/flowsheets/full_treatment_train/flowsheet_components/chemistry/tests/test_posttreatment.py

 @pytest.mark.component
 def test_ideal_naocl_chlorination_full_block():
    model = run_chlorination_block_example(fix_free_chlorine=True)
    assert model.fs.ideal_naocl_mixer_unit.dosing_rate.value == pytest.approx(
-        0.9504457542440085e-6, rel=1e-3
+        1.7296113311683092e-09, rel=1e-3


3 order of magnitude change--not sure if this is correct due to inaccuracies in the original values used that were corrected or if this is an error itself. Without a deep dive into verifying replaced values, my guess is that this result is due to the former.

dh_rxn_ref was also corrected by three orders of magnitude above.

There's also other changes to how the solubility equilibrium constraint is implemented in IDAES IDAES/idaes-pse#1197

OK, so due to the first reason I mentioned. Thanks for confirming.

adam-a-a

Just a couple comments, but overall, should be good to go.

bknueven · 2023-08-18T16:12:43Z

docs/how_to_guides/how_to_use_a_property_model.rst

+   WARNING: model contains export suffix 'fs.state_block[0].scaling_factor' that
+   contains 4 component keys that are not exported as part of the NL file.
+   Skipping.
+   WARNING: model contains export suffix 'fs.state_block[0].scaling_factor' that
+   contains 4 component keys that are not exported as part of the NL file.
+   Skipping.


It's just in the test -- so the reader won't see it.

I talked with @andrewlee94 about this a few weeks ago and there is not a workaround nor is one planned (IDAES and Pyomo are implementing a new model scaling framework)

watertap/examples/chemistry/tests/test_pure_water_pH.py

bknueven · 2023-08-18T16:15:45Z

.github/workflows/checks.yml

@@ -304,4 +300,4 @@ jobs:
        pyomo build-extensions || python -c "from pyomo.contrib.pynumero.asl import AmplInterface; exit(0) if AmplInterface.available() else exit(1)"
    - name: Run pytest
      run: |
-        pytest --pyargs watertap -k 'not nf_dspmde.nf_ui'
+        pytest --pyargs watertap -k 'not (nf_dspmde.nf_ui or nf_dspmde.nf_with_bypass_ui)'


Note -- this only applies to the macOS tests

bknueven · 2023-08-18T16:26:22Z

...s/flowsheets/full_treatment_train/flowsheet_components/chemistry/tests/test_posttreatment.py

 @pytest.mark.component
 def test_ideal_naocl_chlorination_full_block():
    model = run_chlorination_block_example(fix_free_chlorine=True)
    assert model.fs.ideal_naocl_mixer_unit.dosing_rate.value == pytest.approx(
-        0.9504457542440085e-6, rel=1e-3
+        1.7296113311683092e-09, rel=1e-3


dh_rxn_ref was also corrected by three orders of magnitude above.

There's also other changes to how the solubility equilibrium constraint is implemented in IDAES IDAES/idaes-pse#1197

adam-a-a

LGTM

ksbeattie added the Priority:Normal Normal Priority Issue or PR label Feb 23, 2023

ksbeattie assigned bknueven Feb 23, 2023

Revert "switch to nl_v1 (for now)"

57eea20

This reverts commit f062086.

bknueven force-pushed the issue929 branch from 0f8bf27 to 57eea20 Compare March 30, 2023 19:33

bknueven and others added 16 commits March 30, 2023 15:13

doing some debugging

c02dec2

Merge branch 'main' into issue929

e321ead

Modify and rescaled following vars and constraints for chemistry test…

6a209fd

…s, passed log_k_eq_constraint equilibrium_constraint for both solubility product and common eqbm mole_frac_phase_comp

Update tests for chem_scaling_utils modifications

9c41335

Enthalpy unit corrected for EDB data

09ac4c8

chem_scaling_utils updated for corrected enthalpy

c31cd09

Temperature bound modified to avoid breaking out the solvent critical…

d16fc70

… temprature Enthalpy unit corrected

Correct enthalpy units and update assertion in all tests, all pass

21b5c69

Revert "switch to nl_v1 (for now)"

15b4f09

This reverts commit f062086.

doing some debugging

4ca862c

Modify and rescaled following vars and constraints for chemistry test…

4ed5c2e

…s, passed log_k_eq_constraint equilibrium_constraint for both solubility product and common eqbm mole_frac_phase_comp

Update tests for chem_scaling_utils modifications

216f7f4

Enthalpy unit corrected for EDB data

c6011bc

chem_scaling_utils updated for corrected enthalpy

ae9a156

Temperature bound modified to avoid breaking out the solvent critical…

f358836

… temprature Enthalpy unit corrected

Correct enthalpy units and update assertion in all tests, all pass

a54427c

bknueven force-pushed the issue929 branch from e321ead to a54427c Compare June 7, 2023 22:05

Xinhong Liu and others added 7 commits June 15, 2023 13:07

Chemistry tests updated after adding mutable parameters to reformulat…

4be6732

…ed solubility_product

Correct TNK to use components updated in anaerobic environment, break…

f1035c4

… down as an additional variable Use smooth_max from idaes utility and other minor improvements

Update documentation for asm1_adm1 translator

7cd9b05

Typo/equations corrected for asm1_adm1 translator documentation

b207563

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

bc2e2a7

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

0aab640

Merge branch 'lxhowl_issue929' into issue929

1f64faa

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

3d90146

bknueven requested review from adam-a-a, TimBartholomew and lbianchi-lbl August 9, 2023 02:29

Merge branch 'main' into issue929

464e57b

TimBartholomew approved these changes Aug 10, 2023

View reviewed changes

bknueven and others added 3 commits August 17, 2023 12:26

Merge branch 'main' into issue929

31bd11b

pydantic<2

b84c490

Merge branch 'issue929' of github.com:bknueven/proteuslib into issue929

9d903f8

Merge branch 'main' into issue929

edf0d35

adam-a-a reviewed Aug 18, 2023

View reviewed changes

watertap/examples/chemistry/tests/test_pure_water_pH.py Outdated Show resolved Hide resolved

Update watertap/examples/chemistry/tests/test_pure_water_pH.py

42a4803

delete commented debugging code

adam-a-a reviewed Aug 18, 2023

View reviewed changes

watertap/examples/chemistry/tests/test_solids.py Outdated Show resolved Hide resolved

Update watertap/examples/chemistry/tests/test_solids.py

948d410

adam-a-a reviewed Aug 18, 2023

View reviewed changes

run black

52efaf5

bknueven commented Aug 18, 2023

View reviewed changes

bknueven and others added 4 commits August 18, 2023 10:27

remove debugging statement

fd8cca4

Merge branch 'main' into issue929

64a4e03

cleanup pylint failure from watertap-org#1091

1a130cb

Merge branch 'main' into issue929

0adcc9c

adam-a-a enabled auto-merge (squash) August 19, 2023 16:56

adam-a-a approved these changes Aug 19, 2023

View reviewed changes

adam-a-a merged commit 537279d into watertap-org:main Aug 19, 2023
23 checks passed

bknueven deleted the issue929 branch September 22, 2023 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update IDAES requirement to 2.2.0.dev0.watertap.23.08.03 and other stability improvements #930

Update IDAES requirement to 2.2.0.dev0.watertap.23.08.03 and other stability improvements #930

bknueven commented Feb 17, 2023 •

edited

Loading

TimBartholomew left a comment

adam-a-a commented Aug 18, 2023

adam-a-a Aug 18, 2023

bknueven Aug 18, 2023

adam-a-a Aug 18, 2023

lbianchi-lbl Aug 18, 2023 •

edited

Loading

bknueven Aug 18, 2023

adam-a-a Aug 18, 2023

adam-a-a Aug 18, 2023

bknueven Aug 18, 2023

adam-a-a Aug 19, 2023

adam-a-a left a comment

bknueven Aug 18, 2023

bknueven Aug 18, 2023

bknueven Aug 18, 2023

adam-a-a left a comment

Update IDAES requirement to 2.2.0.dev0.watertap.23.08.03 and other stability improvements #930

Update IDAES requirement to 2.2.0.dev0.watertap.23.08.03 and other stability improvements #930

Conversation

bknueven commented Feb 17, 2023 • edited Loading

Fixes #929, Fixes #1072

Changes proposed in this PR:

Legal Acknowledgement

TimBartholomew left a comment

Choose a reason for hiding this comment

adam-a-a commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbianchi-lbl Aug 18, 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

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

adam-a-a left a comment

Choose a reason for hiding this comment

bknueven commented Feb 17, 2023 •

edited

Loading

lbianchi-lbl Aug 18, 2023 •

edited

Loading