Scaling hints by dallan-keylogic · Pull Request #1649 · IDAES/idaes-pse

dallan-keylogic · 2025-08-12T20:53:28Z

Summary/Motivation:

While the old scaling toolbox supported setting scaling factors for both Vars and Expressions, the Pyomo team indicated to us that we were abusing the scaling suffix by setting scaling factors for Expressions because they would not be exported to the NLP. However, using the NominalValueExpressionWalker for the sorts of giga-expressions that are regularly found in IDAES thermodynamic packages can be dangerous. As a result, I implemented a system of "scaling hints" that could be assigned to named Expressions, sequestering them in the scaling_hint suffix.

Additionally, I modified the call_submodel_scaler_method so that it passes the parent Scaler's configuration and map of submodel scalers to the child Scaler objects. Note that some child scalers were not written expecting the submodel_scalers argument, so this does break backwards compatibility, but I doubt that there has been any major adoption of these new scaling tools.

This resolves point (1) in #1648.

Changes proposed in this PR:

Add system of scaling hints for Expressions
Pass config and submodel_scalers to child scalers in call_submodel_scaler_method of a parent scaler
Add scale_constraint_by_component method to CustomScalerBase in order to take the scaling factor from a Var or Expression and assign it to a constraint.
Modify scale_variable_from_default in order to avoid silent failures, as well as to make it clear to the user when their input into scaling is necessary.
Raise an exception if people try to set scaling factors on indexed components (this is related to point (10) in Outstanding issues with the new scaling tools #1648)

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

Minor comments, but nothing that should hold this back

MarcusHolly · 2025-08-14T16:21:58Z

idaes/core/scaling/custom_scaler_base.py

+            raise TypeError(f"{constraint} is not a constraint (or is indexed).")
+        self._scale_component_by_default(component=constraint, overwrite=overwrite)
+
+    def get_expression_nominal_value(self, expression):


Is there not a better way to combine/differentiate the three nominal value functions?

We have get_expression_nominal_values deprecated, and it can be removed in the next release. The difference between get_sum_terms_nominal_values and get_expression_nominal_value is that the former returns a list while the latter returns a float.

MarcusHolly · 2025-08-14T16:26:32Z

idaes/core/scaling/custom_scaler_base.py

+    def propagate_state_data_scaling(
+        self, target_state_data, source_state_data, overwrite: bool = False
+    ):
+        """
+        Propagate scaling of state variables from one StateBlockData to another.
+
+        Args:
+            target_state_data: StateBlockData to set scaling factors on
+            source_state_data: StateBlockData to use as source for scaling factors
+            overwrite: whether to overwrite existing scaling factors
+
+        Returns:
+            None
+        """
+        target_vars = target_state_data.define_state_vars()
+        source_vars = source_state_data.define_state_vars()
+
+        for state, var in target_vars.items():
+            for vidx, vardata in var.items():
+                self.scale_variable_by_component(
+                    target_variable=vardata,
+                    scaling_component=source_vars[state][vidx],
+                    overwrite=overwrite,
+                )


Similar point with this - is there a simple way to combine this with the existing function?

Is the proposed behavior to have propagate_state_scaling check whether the inputs are indexed or unindexed, then handle those cases appropriately?

^^^ I think this is right. The function signature and return value are the same, so might as well allow it to handle both types of inputs - that being said, I would make sure that the documentation clearly explains it.

MarcusHolly · 2025-08-14T16:27:14Z

idaes/core/scaling/scaling_base.py

 CONFIG.declare(
    "max_variable_scaling_factor",
    ConfigValue(
+        # default=float("inf"),


Better to just delete these default values?

I really want to get rid of these max and min values, because we're going to need to override them in pretty much every scaler in PrOMMiS, but they're required for the Autoscalers to function. I can certainly get rid of these comments, though.

strahlw

First pass of the PR (it is very large) - there are certainly nuances that I am missing, but I left some comments of things that popped out.

idaes/core/scaling/custom_scaler_base.py

strahlw · 2025-08-14T17:50:29Z

idaes/core/scaling/custom_scaler_base.py

+            None
+        """
+        if not (isinstance(variable, VarData) or isinstance(variable, ExpressionData)):
+            raise TypeError(f"{variable} is not a variable/expression (or is indexed).")


See comment on scale_variable_by_units. I think it would be beneficial to have clearer feedback to the user if they encounter these types of errors.

strahlw · 2025-08-14T19:05:10Z

idaes/core/scaling/custom_scaler_base.py

+    def propagate_state_data_scaling(
+        self, target_state_data, source_state_data, overwrite: bool = False
+    ):
+        """
+        Propagate scaling of state variables from one StateBlockData to another.
+
+        Args:
+            target_state_data: StateBlockData to set scaling factors on
+            source_state_data: StateBlockData to use as source for scaling factors
+            overwrite: whether to overwrite existing scaling factors
+
+        Returns:
+            None
+        """
+        target_vars = target_state_data.define_state_vars()
+        source_vars = source_state_data.define_state_vars()
+
+        for state, var in target_vars.items():
+            for vidx, vardata in var.items():
+                self.scale_variable_by_component(
+                    target_variable=vardata,
+                    scaling_component=source_vars[state][vidx],
+                    overwrite=overwrite,
+                )


^^^ I think this is right. The function signature and return value are the same, so might as well allow it to handle both types of inputs - that being said, I would make sure that the documentation clearly explains it.

strahlw · 2025-08-14T19:32:17Z

idaes/core/scaling/util.py

+        _log.debug(f"Created new scaling hint suffix for {_filter_unknown(blk)}")
+        sfx = blk.scaling_hint = Suffix(direction=Suffix.EXPORT)
+
+    return sfx


There's duplicate code in 84-94 and 121-131. I suggest breaking it out into a separate function and having an argument that determines whether it uses "hints" or "factors" in the type error message.

strahlw · 2025-08-14T19:39:34Z

idaes/core/scaling/util.py

        if comp is not None:
+            if valid_types is not None and not any(
+                [isinstance(comp, cls) for cls in valid_types]
+            ):


You can just use the generator here, no need to encapsulate in a list

strahlw · 2025-08-14T20:17:20Z

idaes/models/unit_models/tests/test_equilibrium_reactor.py

        jac, _ = get_jacobian(sm, scaled=False)
        assert (jacobian_cond(jac=jac, scaled=False)) == pytest.approx(
-            4.987e05, rel=1e-3
+            5.445e05, rel=1e-3


I take it this changes because we now scale fixed variables for the "nominal" values.

strahlw · 2025-08-14T20:19:20Z

idaes/core/scaling/tests/test_nominal_value_walker.py


        # Nominal value should be value
-        assert NominalValueExtractionVisitor().walk_expression(expr=m.var) == [-1]
+        assert NominalValueExtractionVisitor().walk_expression(expr=m.var) == [-4]


Assuming this is an understood/intended change -> looking further down in the PR it looks like a "fundamental" change going on is no longer using fixed values as "nominal" values. If this is a correct understanding, please update the comment on line 130.

strahlw · 2025-08-14T21:04:59Z

idaes/core/scaling/tests/test_scaling_profiler.py

+Inverse Sum              || 9.077E+15 | Failed 200 || 1.502E+06 | Solved 6  
+Inverse Root Sum Squares || 1.083E+16 | Failed 200 || 9.600E+05 | Solved 6  
+Inverse Maximum          || 1.066E+16 | Failed 200 || 7.846E+05 | Solved 6  
+Inverse Minimum          || 7.543E+18 | Failed 200 || 5.599E+12 | Solved 16 


Is it clear why only the inverse minimum changed for the "perfect scaling" case. That seems a little odd to me (although percentage-wise it is a small change). Also, the user-scaling cases fail many times more - is this understood behavior, or just an observed response? (Does it need to be understood?)

codecov · 2025-08-14T21:56:28Z

Codecov Report

❌ Patch coverage is 89.44724% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.02%. Comparing base (3b4ebdb) to head (0318102).
⚠️ Report is 38 commits behind head on scaling_toolbox.

Files with missing lines	Patch %	Lines
idaes/core/scaling/util.py	88.59%	5 Missing and 8 partials ⚠️
idaes/core/scaling/scaling_base.py	66.66%	5 Missing and 2 partials ⚠️
idaes/core/scaling/custom_scaler_base.py	98.41%	1 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           scaling_toolbox    #1649      +/-   ##
===================================================
+ Coverage            77.01%   77.02%   +0.01%     
===================================================
  Files                  395      395              
  Lines                63555    63689     +134     
  Branches             10365    10405      +40     
===================================================
+ Hits                 48944    49056     +112     
- Misses               12171    12184      +13     
- Partials              2440     2449       +9

☔ 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.

* rescue files from overloaded git branch * fix due to api tweaks * run black * forgot to add a file * fix test errors * pin coolprop version * update version, disable superancillaries * run black * respond to Marcus's feedback * getting close * address Will's comments * tests for set_scaling_factor * support for unions in python 3.9 * testing the scaling profiler is way too fragile * modify test to be less fragile * remove pdb

dallan-keylogic added 3 commits August 12, 2025 16:15

rescue files from overloaded git branch

9741903

fix due to api tweaks

b744bac

run black

a98d142

dallan-keylogic requested review from agarciadiego, bpaul4 and lbianchi-lbl as code owners August 12, 2025 20:53

dallan-keylogic requested a review from MAZamarripa August 12, 2025 20:53

dallan-keylogic added 3 commits August 12, 2025 16:55

forgot to add a file

958ada7

fix test errors

72a4e17

pin coolprop version

0e83eed

MarcusHolly approved these changes Aug 14, 2025

View reviewed changes

Merge branch 'coolprop_fix' into scaling-hints-limited

9273044

ksbeattie added the Priority:High High Priority Issue or PR label Aug 14, 2025

ksbeattie assigned dallan-keylogic Aug 14, 2025

dallan-keylogic added 3 commits August 14, 2025 15:26

update version, disable superancillaries

8ffb137

Merge branch 'coolprop_fix' into scaling-hints-limited

8772e06

run black

6af3df3

strahlw reviewed Aug 14, 2025

View reviewed changes

respond to Marcus's feedback

e4e06d5

dallan-keylogic requested a review from ksbeattie as a code owner August 14, 2025 21:07

getting close

15942e7

dallan-keylogic added 2 commits August 15, 2025 11:10

address Will's comments

fb0c68c

Merge branch 'main' into scaling-hints-limited

afaf1c6

dallan-keylogic changed the base branch from main to scaling_toolbox August 15, 2025 15:18

dallan-keylogic added 5 commits August 15, 2025 11:30

tests for set_scaling_factor

8e5c6ef

support for unions in python 3.9

d5ab66f

testing the scaling profiler is way too fragile

b3ac646

modify test to be less fragile

142d773

remove pdb

0318102

dallan-keylogic merged commit ff5c700 into IDAES:scaling_toolbox Aug 15, 2025
39 checks passed

dallan-keylogic deleted the scaling-hints-limited branch August 15, 2025 18:20

Conversation

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

Uh oh!

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

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!

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!

strahlw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dallan-keylogic commented Aug 12, 2025 •

edited

Loading

codecov bot commented Aug 14, 2025 •

edited

Loading