Mixer Scaler by dallan-keylogic · Pull Request #1676 · IDAES/idaes-pse

dallan-keylogic · 2025-10-01T19:39:21Z

Summary/Motivation:

Scaler object for the mixer unit model. Depends on #1673.

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 · 2025-10-01T20:35:53Z

Codecov Report

❌ Patch coverage is 79.72973% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.21%. Comparing base (0a8eed3) to head (6f73d07).
⚠️ Report is 24 commits behind head on scaling_toolbox.

Files with missing lines	Patch %	Lines
idaes/core/base/control_volume_base.py	75.75%	8 Missing ⚠️
idaes/models/unit_models/mixer.py	82.92%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           scaling_toolbox    #1676      +/-   ##
===================================================
- Coverage            77.21%   77.21%   -0.01%     
===================================================
  Files                  395      395              
  Lines                63859    63923      +64     
  Branches             10606    10627      +21     
===================================================
+ Hits                 49306    49355      +49     
- Misses               12065    12079      +14     
- Partials              2488     2489       +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MarcusHolly

Seems like the only significant changes are taking place in the mixer.py and test_mixer.py files, right? I'm assuming the rest is carryover from #1673

MarcusHolly · 2025-10-14T17:36:00Z

idaes/models/unit_models/tests/test_mixer.py

+        for var in model.conc_mass_comp.values():
+            self.scale_variable_by_default(var, overwrite=overwrite)


What happens if you do this, but one or more of the components doesn't have a default scaling factor. Would this still run successfully? If so, what scaling factor gets set for the unspecified component? If this fails, how does it fail? Warning/error message and, if so, is this tested?

It raises an exception:

raise ValueError( "This scaler requires the user to provide a default " f"scaling factor for {component}, but no default scaling " "factor was set." )

MarcusHolly · 2025-10-14T17:37:22Z

idaes/models/unit_models/tests/test_mixer.py

                self[k].hso4_dissociation.deactivate()


+class LeachSolutionScaler(CustomScalerBase):


Why is this being built in this file?

We needed a test case with inherent reactions. I considered pulling both this scaler and the parameter block it's designed to scale out of this test file, but I saw that it had non-SI base units and didn't want to spread that further in the IDAES code base.

In that case, could you just leave an in-line comment/TODO note stating what you've said here.

MarcusHolly · 2025-10-14T17:40:36Z

idaes/models/unit_models/tests/test_mixer.py

+    @pytest.mark.component
+    @pytest.mark.solver
+    def test_no_numerical_warnings(self, model):
+        # TODO revisit once the diagnostics toolbox takes scaling into account


Just out of curiosity, is this something that is being actively worked on? I think it'd be amazing if the toolbox could take scaling into account, but I recall Andrew mentioning that this was easier said than done.

It's actually straightforward to do now. We had issues when we had scale_model=False in the writer config, so it was unclear whether the user_scaling option was being used for IPOPT. Now we have scale_model=True, so we can always return the scaled version of the Jacobian, constraint residuals, and variable distance from bounds.

bpaul4 · 2025-10-14T22:10:35Z

idaes/core/base/control_volume1d.py

+                    model.material_flow_dx[idx], v, overwrite=overwrite
+                )
+
+        # TODO elemental flows


Will this be part of another PR?

bpaul4 · 2025-10-14T22:11:15Z

idaes/core/base/control_volume1d.py

+                v,
+            ) in model._enthalpy_flow.items():  # pylint: disable=protected-access
+                # Normalized domain, so scale should be the same as flow
+                # TODO is this correct?


Does this still need to be addressed?

Yes, but now isn't the time to do it. I need to take some time to sit down with the advection equations and explore the conditioning of the solution, both in the case of a single ControlVolume1D and a counter-current unit model such as the HeatExchanger1D. The current scaling is adequate for many purposes.

idaes/core/base/control_volume_base.py

bpaul4 · 2025-10-14T22:27:05Z

idaes/core/base/tests/test_control_volume_0d_scaler_object.py



+def approx(x):
+    return pytest.approx(x, rel=1e-15, abs=0)


This tolerance is extremely tight, could there be failures in the future from this?

Here we're trying to match scaling factors like 1/10 * 1/5 * 3/10. Floating point arithmetic is not associative, so (1/10 * 1/5)*(3/10) !=(1/10) * (1/5*3/10) . However, it will be within a relative tolerance of machine epsilon, which is 2.22e-16 for double precision arithmetic in Python. So no, there shouldn't be failures in the future from this.

Co-authored-by: Brandon Paul <[email protected]>

idaes/models/unit_models/tests/test_mixer.py

MarcusHolly

LGTM

@bpaul4

* additional test for propagating scaling factors within a block * Control Volume 1D scaler * phase material balance cv0d * test for submodel scaler for blockdata * Tests for flow direction attribute * black and typo * remove unused variables * protected property error message depends on python version * test no override * test propagate scaling factors * additional test coverage * Mixer scaler * test with inherent reactions * test inlet_blocks * Pylint * Apply suggestion from @bpaul4 Co-authored-by: Brandon Paul <[email protected]> * default_scaler attribute should live on indexed block * Apply suggestion from @dallan-keylogic * Apply suggestion from @dallan-keylogic --------- Co-authored-by: Brandon Paul <[email protected]>

dallan-keylogic added 16 commits September 25, 2025 10:48

additional test for propagating scaling factors within a block

82a1e84

Control Volume 1D scaler

d27e0f4

phase material balance cv0d

3483fb3

test for submodel scaler for blockdata

557eed5

Merge branch 'scaling_toolbox' into control_volume_1D_scaler

96de1a0

Tests for flow direction attribute

e5582d4

black and typo

929f3a6

remove unused variables

dd900f6

protected property error message depends on python version

be2d0b1

test no override

05b4abd

test propagate scaling factors

0d86465

additional test coverage

321fae0

Merge branch 'control_volume_1D_scaler' into mixer_scaler

ae0406a

Mixer scaler

7e10d42

test with inherent reactions

f82096d

test inlet_blocks

dc35f30

dallan-keylogic requested review from agarciadiego and bpaul4 as code owners October 1, 2025 19:39

Pylint

168b55d

MarcusHolly reviewed Oct 14, 2025

View reviewed changes

bpaul4 reviewed Oct 14, 2025

View reviewed changes

dallan-keylogic and others added 4 commits October 15, 2025 15:13

Merge branch 'scaling_toolbox' into mixer_scaler

5be56ea

Apply suggestion from @bpaul4

04b271c

Co-authored-by: Brandon Paul <[email protected]>

default_scaler attribute should live on indexed block

fd525a9

Merge branch 'mixer_scaler_merge' into mixer_scaler

5bf1717

dallan-keylogic commented Oct 15, 2025

View reviewed changes

idaes/models/unit_models/tests/test_mixer.py Show resolved Hide resolved

Apply suggestion from @dallan-keylogic

a2ef43f

dallan-keylogic commented Oct 15, 2025

View reviewed changes

idaes/models/unit_models/tests/test_mixer.py Outdated Show resolved Hide resolved

Apply suggestion from @dallan-keylogic

6f73d07

MarcusHolly approved these changes Oct 16, 2025

View reviewed changes

dallan-keylogic merged commit 88a602b into IDAES:scaling_toolbox Oct 17, 2025
55 checks passed

		for var in model.conc_mass_comp.values():
		self.scale_variable_by_default(var, overwrite=overwrite)

		self[k].hso4_dissociation.deactivate()


		class LeachSolutionScaler(CustomScalerBase):

Conversation

dallan-keylogic commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary/Motivation:

Legal Acknowledgement

Uh oh!

codecov bot commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MarcusHolly left a comment

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!

MarcusHolly Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarcusHolly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dallan-keylogic commented Oct 1, 2025 •

edited

Loading

codecov bot commented Oct 1, 2025 •

edited

Loading

MarcusHolly Oct 15, 2025 •

edited

Loading