RO Scaling improvements #1704

dallan-keylogic · 2025-12-02T23:40:08Z

Summary/Motivation:

Add additional scaling for various unit models, mostly involving RO. Depends on #1695.

Also, the unit test harness is slightly modified. The check for "badly scaled variables" can be bypassed, and an optional check for the model condition number is added instead.

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.

adam-a-a · 2025-12-02T23:49:57Z

watertap/core/membrane_channel1d.py


    def calculate_scaling_factors(self):
+        if iscale.get_scaling_factor(self.area) is None:
+            iscale.set_scaling_factor(self.area, 100)


Should we also display a warning to the user that we are doing this since so scaling factor was provided?

Do the old scaling tools really need additional warnings to spam the user's logs?

In the new scaling tools, I'm going to raise an exception instead, so they can no longer ignore it.

@dallan-keylogic in the new scaling tools, could you think about perhaps a setting where the user could choose to have exceptions raised or bypass (with warnings if you prefer)? The default would be to raise exceptions, but it'd be nice to have an escape route for devs (for whatever exploratory reasons someone might have).

adam-a-a · 2025-12-02T23:52:31Z

watertap/core/membrane_channel_base.py

                if iscale.get_scaling_factor(v) is None:
                    iscale.set_scaling_factor(v, 1)

+        # TODO why did this file have zero scaling factors for constraints


@bknueven can probably give a more informative response, but the line of thinking was that scaling all variables should suffice in most cases without the need to scale constraints. However, we know that this isn't always the case, as we've experienced particularly when dealing with getting "complex" models to converge.

adam-a-a · 2025-12-02T23:54:07Z

watertap/flowsheets/full_water_resource_recovery_facility/test/test_BSM2_P_extension.py

-        warnings, next_steps = dt._collect_numerical_warnings()
-        assert len(warnings) == 3
+        warnings, _ = dt._collect_numerical_warnings()
+        assert len(warnings) == 1


adam-a-a · 2025-12-02T23:56:14Z

watertap/flowsheets/full_water_resource_recovery_facility/BSM2.py


-    initializer = BlockTriangularizationInitializer()
+    initializer = BlockTriangularizationInitializer(
+        calculate_variable_options={"eps": 2e-8}, skip_final_solve=True


how is eps applied through calculate_variable_options?

It is the tolerance used to check whether the constraint residual is small enough to consider the constraint "satisfied". When solving the flowsheet, I encountered a problem where it couldn't reduce the residual of some enthalpy constraint (i.e., a constraint that needs a scaling factor <<1) to less than 1e-8. Due to Pyomo/pyomo#3785 , it does not take the constraint's scaling factor into account when calculating this residual. Using a constraint_scaling_transformation would allow scaling to be taken into account, but we wanted to move away from that in the new scaling tools.

adam-a-a · 2025-12-02T23:59:34Z

watertap/flowsheets/full_water_resource_recovery_facility/BSM2_P_extension.py

+            # TODO is it intentional that we're setting scaling factors for some constraints
+            # but doing constraint scaling transforms for others?


Not intentional--traditionally we applied constraint scaling transforms.

this appears to contradict @MarcusHolly in #1695

adam-a-a · 2025-12-03T00:01:58Z

watertap/property_models/multicomp_aq_sol_prop_pack.py

+                    b.flow_mass_phase_comp[p, j] / b.params.mw_comp[j]
+                    == b.flow_mol_phase_comp[p, j]


Why opt for this form?

This is changed due to how the WaterTap scaling utilities work. Because the constraint is named eq_flow_mol_phase_comp, it is automatically scaled using the scaling factor for flow_mol_phase_comp. However, as originally written, it ought to be scaled instead like flow_mass_phase_comp. This slight modification of the constraint is easier than messing with that workflow.

adam-a-a · 2025-12-03T00:03:39Z

watertap/property_models/tests/test_multicomp_aq_sol_prop_pack.py

-        ] == pytest.approx(12120000, rel=1e-3)
+        ] == pytest.approx(4.309e07, rel=1e-3)
        assert m.fs.stream[0].scaling_factor[
            m.fs.stream[0].eq_elec_mobility_phase_comp["Liq", "Na_+"]
-        ] == pytest.approx(19320000, rel=1e-3)
+        ] == pytest.approx(8.617e07, rel=1e-3)
        assert m.fs.stream[0].scaling_factor[
            m.fs.stream[0].eq_elec_mobility_phase_comp["Liq", "Cl_-"]
-        ] == pytest.approx(12660000, rel=1e-3)
+        ] == pytest.approx(8.617e07, rel=1e-3)
        assert m.fs.stream[0].scaling_factor[
            m.fs.stream[0].eq_elec_mobility_phase_comp["Liq", "Mg_2+"]
-        ] == pytest.approx(18200000, rel=1e-3)
+        ] == pytest.approx(4.309e07, rel=1e-3)


Unclear why we are getting relatively significant changes in these test values

It's clearer now.

adam-a-a · 2025-12-03T00:08:32Z

watertap/unit_models/tests/test_nanofiltration_0D.py

-        ] == pytest.approx(0.01866, rel=1e-3)
+        ] == pytest.approx(0.1, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Ca_2+"]
-        ] == pytest.approx(104.7, rel=1e-3)
+        ] == pytest.approx(1e3, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "SO4_2-"]
-        ] == pytest.approx(44.94, rel=1e-3)
+        ] == pytest.approx(1e2, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Mg_2+"]
-        ] == pytest.approx(1.722, rel=1e-3)
+        ] == pytest.approx(10, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Na_+"]
-        ] == pytest.approx(2.068, rel=1e-3)
+        ] == pytest.approx(10, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Cl_-"]
-        ] == pytest.approx(0.6174, rel=1e-3)
+        ] == pytest.approx(1, rel=1e-3)

        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "H2O"]
-        ] == pytest.approx(0.0233193, rel=1e-5)
+        ] == pytest.approx(0.1, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "Ca_2+"]
        ] == pytest.approx(0.004, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "SO4_2-"]
        ] == pytest.approx(0.00960, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "Mg_2+"]
        ] == pytest.approx(0.00240, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "Na_+"]
        ] == pytest.approx(0.0023, rel=1e-3)

        assert (
            mcas_case.fs.unit.scaling_factor[
                mcas_case.fs.unit.permeate_electronegativity[0.0]
            ]
            == 1
        )
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.retentate_pressure_balance[0.0]
-        ] == pytest.approx(0.0000025, rel=1e-3)
+        ] == pytest.approx(1e-5, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.retentate_temperature_equality[0.0]
-        ] == pytest.approx(0.003354, rel=1e-3)
+        ] == pytest.approx(1e-2, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.permeate_temperature_equality[0.0]
-        ] == pytest.approx(0.003354, rel=1e-3)
+        ] == pytest.approx(1e-2, rel=1e-3)


These changes seem significant

These changes weren't a result of this PR, but instead #1695. They're due to a change in how the nominal value expression walker works. Previously it used 1/value(var) if var was fixed, but now it uses 1/scaling_factor[var] regardless of whether or not var is fixed.

adam-a-a · 2025-12-03T00:09:38Z

watertap/unit_models/tests/test_reverse_osmosis_1D.py

+        self.skip_badly_scaled_vars = True
+        self.condition_number = 3.9140627e8  # Was previously 4.8157167e15


these seem like great improvements!

dallan-keylogic · 2025-12-02T23:42:11Z

watertap/property_models/multicomp_aq_sol_prop_pack.py

+                    b.flow_mass_phase_comp[p, j] / b.params.mw_comp[j]
+                    == b.flow_mol_phase_comp[p, j]


This is changed due to how the WaterTap scaling utilities work. Because the constraint is named eq_flow_mol_phase_comp, it is automatically scaled using the scaling factor for flow_mol_phase_comp. However, as originally written, it ought to be scaled instead like flow_mass_phase_comp. This slight modification of the constraint is easier than messing with that workflow.

dallan-keylogic · 2025-12-02T23:51:10Z

watertap/core/membrane_channel1d.py


    def calculate_scaling_factors(self):
+        if iscale.get_scaling_factor(self.area) is None:
+            iscale.set_scaling_factor(self.area, 100)


Do the old scaling tools really need additional warnings to spam the user's logs?

In the new scaling tools, I'm going to raise an exception instead, so they can no longer ignore it.

dallan-keylogic · 2025-12-03T14:54:41Z

watertap/flowsheets/full_water_resource_recovery_facility/BSM2.py


-    initializer = BlockTriangularizationInitializer()
+    initializer = BlockTriangularizationInitializer(
+        calculate_variable_options={"eps": 2e-8}, skip_final_solve=True


It is the tolerance used to check whether the constraint residual is small enough to consider the constraint "satisfied". When solving the flowsheet, I encountered a problem where it couldn't reduce the residual of some enthalpy constraint (i.e., a constraint that needs a scaling factor <<1) to less than 1e-8. Due to Pyomo/pyomo#3785 , it does not take the constraint's scaling factor into account when calculating this residual. Using a constraint_scaling_transformation would allow scaling to be taken into account, but we wanted to move away from that in the new scaling tools.

dallan-keylogic · 2025-12-03T14:55:55Z

watertap/property_models/multicomp_aq_sol_prop_pack.py

+                    b.flow_mass_phase_comp[p, j] / b.params.mw_comp[j]
+                    == b.flow_mol_phase_comp[p, j]


This is changed due to how the WaterTap scaling utilities work. Because the constraint is named eq_flow_mol_phase_comp, it is automatically scaled using the scaling factor for flow_mol_phase_comp. However, as originally written, it ought to be scaled instead like flow_mass_phase_comp. This slight modification of the constraint is easier than messing with that workflow.

dallan-keylogic · 2025-12-03T15:04:07Z

watertap/unit_models/tests/test_nanofiltration_0D.py

-        ] == pytest.approx(0.01866, rel=1e-3)
+        ] == pytest.approx(0.1, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Ca_2+"]
-        ] == pytest.approx(104.7, rel=1e-3)
+        ] == pytest.approx(1e3, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "SO4_2-"]
-        ] == pytest.approx(44.94, rel=1e-3)
+        ] == pytest.approx(1e2, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Mg_2+"]
-        ] == pytest.approx(1.722, rel=1e-3)
+        ] == pytest.approx(10, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Na_+"]
-        ] == pytest.approx(2.068, rel=1e-3)
+        ] == pytest.approx(10, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.material_balances[0.0, "Cl_-"]
-        ] == pytest.approx(0.6174, rel=1e-3)
+        ] == pytest.approx(1, rel=1e-3)

        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "H2O"]
-        ] == pytest.approx(0.0233193, rel=1e-5)
+        ] == pytest.approx(0.1, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "Ca_2+"]
        ] == pytest.approx(0.004, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "SO4_2-"]
        ] == pytest.approx(0.00960, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "Mg_2+"]
        ] == pytest.approx(0.00240, rel=1e-5)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.separation_constraint[0.0, "Liq", "Na_+"]
        ] == pytest.approx(0.0023, rel=1e-3)

        assert (
            mcas_case.fs.unit.scaling_factor[
                mcas_case.fs.unit.permeate_electronegativity[0.0]
            ]
            == 1
        )
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.retentate_pressure_balance[0.0]
-        ] == pytest.approx(0.0000025, rel=1e-3)
+        ] == pytest.approx(1e-5, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.retentate_temperature_equality[0.0]
-        ] == pytest.approx(0.003354, rel=1e-3)
+        ] == pytest.approx(1e-2, rel=1e-3)
        assert mcas_case.fs.unit.scaling_factor[
            mcas_case.fs.unit.permeate_temperature_equality[0.0]
-        ] == pytest.approx(0.003354, rel=1e-3)
+        ] == pytest.approx(1e-2, rel=1e-3)


These changes weren't a result of this PR, but instead #1695. They're due to a change in how the nominal value expression walker works. Previously it used 1/value(var) if var was fixed, but now it uses 1/scaling_factor[var] regardless of whether or not var is fixed.

dallan-keylogic added 14 commits November 21, 2025 17:55

Bring WaterTap in line with changes in IDAES scaling

94e7fae

pylint

a700b81

Relative error

9834883

breakdown by platform

b054ca6

1711 got merged

f5e0f4e

trying to install prommis

7acfdf2

that didn't work

dd4ff82

pull from the IDAES release candidate

b0b122d

fix requirement

6feb672

...

f7e5d7b

scaling

61ce99a

document condition numbers

7400030

Merge branch 'document_condition_numbers' into UCI_scaling

9edda78

condition number improving

8870271

adam-a-a reviewed Dec 2, 2025

View reviewed changes

adam-a-a reviewed Dec 3, 2025

View reviewed changes

Forgot to commit final set of changes

765791f

dallan-keylogic commented Dec 3, 2025

View reviewed changes

dallan-keylogic added 2 commits December 3, 2025 10:08

black

5021d2a

linux

725d09e

ksbeattie added the Priority:Normal Normal Priority Issue or PR label Dec 4, 2025

ksbeattie assigned dallan-keylogic Dec 4, 2025

		# TODO is it intentional that we're setting scaling factors for some constraints
		# but doing constraint scaling transforms for others?

		b.flow_mass_phase_comp[p, j] / b.params.mw_comp[j]
		== b.flow_mol_phase_comp[p, j]

		self.skip_badly_scaled_vars = True
		self.condition_number = 3.9140627e8 # Was previously 4.8157167e15

RO Scaling improvements #1704

Are you sure you want to change the base?

RO Scaling improvements #1704

Uh oh!

Conversation

dallan-keylogic commented Dec 2, 2025

Summary/Motivation:

Legal Acknowledgement

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants