Scale Variable from Definition Constraint by dallan-keylogic · Pull Request #1690 · IDAES/idaes-pse

dallan-keylogic · 2025-10-24T14:25:13Z

Summary/Motivation:

Changes scale_variable_from_definition_constraint to take scaling hints into account (which are extremely useful when dealing with enthalpy and energy terms).

Also pins the Pyomo version to 6.9.4 until such time as #1683 is merged.

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.

…nto account

codecov · 2025-10-24T16:54:28Z

Codecov Report

❌ Patch coverage is 94.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.30%. Comparing base (3ddc9c4) to head (da60bd4).
⚠️ Report is 14 commits behind head on scaling_toolbox.

Files with missing lines	Patch %	Lines
idaes/core/scaling/custom_scaler_base.py	94.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           scaling_toolbox    #1690   +/-   ##
================================================
  Coverage            77.30%   77.30%           
================================================
  Files                  395      395           
  Lines                64166    64192   +26     
  Branches             10730    10736    +6     
================================================
+ Hits                 49603    49626   +23     
- Misses               12072    12073    +1     
- Partials              2491     2493    +2

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

bpaul4

Just a few comments.

bpaul4 · 2025-10-24T22:47:17Z

idaes/core/scaling/custom_scaler_base.py

+            safety_mode: Flag about whether or not to screen named expressions to see if
+                "variable" appears in them. Screening them takes longer (especially for
+                extremely deep expression trees), but if we do not screen them we run
+                the risk of removing "variable" from "constraint" entirely.


Why is this true? The screening is not actually deleting components, is it?

The screening is not deleting components, but replace_expressions is building a new expression in which every variable and named expression in the keys of replacement_map is replaced by the expression in the values of replacement_map. Since we're going to be using calculate_variable_from_constraint in order to get the scaling factor, we still need variable to show up in the replaced expression.

bpaul4 · 2025-10-24T22:49:08Z

idaes/core/scaling/custom_scaler_base.py

+                f"Variable {variable} is indexed. Call with VarData "
+                "children instead."


Listing the index keys or "children" might be helpful if this is thrown.

You mean iterate over variable to list every single VarData child? I don't think being bombarded with a list of the form

fs.unit.control_volume.material_holdup[0,0,"H2O"] fs.unit.control_volume.material_holdup[0,0,"NaOH"] fs.unit.control_volume.material_holdup[0,0, "EthylAcetate"] fs.unit.control_volume.material_holdup[0,0, "SodiumAcetate"] fs.unit.control_volume.material_holdup[0,0, "Ethanol"] fs.unit.control_volume.material_holdup[0,0.1,"H2O"] fs.unit.control_volume.material_holdup[0,0.1,"NaOH"] fs.unit.control_volume.material_holdup[0,0.1, "EthylAcetate"] fs.unit.control_volume.material_holdup[0,0.1, "SodiumAcetate"] fs.unit.control_volume.material_holdup[0,0.1, "Ethanol"] etc...

is going to be particularly helpful (unless they don't know the difference between an IndexedVar and VarData in the first place).

bpaul4 · 2025-10-24T22:50:48Z

idaes/core/scaling/custom_scaler_base.py

+            var_set = ComponentSet(var for var in identify_variables(replaced_expr))
+            if not (len(var_set) == 1 and variable in var_set):
+                raise BurntToast(
+                    "Something has gone wrong in the variable replacement scheme. "


Maybe make this line a bit more formal sounding, since it's exposed to users.

Something like "An unexpected failure has occurred in the variable replacement scheme. Pleas open an issue..."

Theoretically, the user should never see this line---we handle the case of variable not appearing in constraint using the variable_in_constraint flag, while safety_mode should make sure that a named expression containing variable is not replaced. If we end up here anyway, it's because some unknown error has occurred, so we can't be more specific with the error message.

bpaul4 · 2025-10-24T22:56:01Z

setup.py

    # Concrete dependencies go in requirements[-dev].txt
    install_requires=[
-        "pyomo >= 6.9.3",
+        "pyomo == 6.9.4",  # Temporary pin to avoid Pylint issues


Should we mark a TODO here, as well?

MarcusHolly · 2025-10-27T13:39:48Z

idaes/core/scaling/custom_scaler_base.py

+            var_set = ComponentSet(var for var in identify_variables(replaced_expr))
+            if not (len(var_set) == 1 and variable in var_set):
+                raise BurntToast(
+                    "Something has gone wrong in the variable replacement scheme. "


Something like "An unexpected failure has occurred in the variable replacement scheme. Pleas open an issue..."

MarcusHolly · 2025-10-27T13:55:09Z

idaes/core/scaling/custom_scaler_base.py

+        """
+        Helper function for scale_variable_by_definition_constraint.
+
+        Walks the constraint to build a map between Variables or Expressions


This doesn't sound right to me. What is meant by "walks"? A synonym for "helps"?

* tweak scale_variable_by_definition_constraint to take scaling hints into account * pin pyomo * unused import * failing test * additional coverage * expressions no hints * respond to reviewer comments * more reviewer comments

dallan-keylogic added 2 commits October 24, 2025 10:19

tweak scale_variable_by_definition_constraint to take scaling hints i…

a1c5a6f

…nto account

pin pyomo

31bea04

dallan-keylogic requested review from MAZamarripa, agarciadiego, bpaul4, ksbeattie and sufikaur as code owners October 24, 2025 14:25

dallan-keylogic changed the base branch from main to scaling_toolbox October 24, 2025 14:25

dallan-keylogic added 2 commits October 24, 2025 10:32

unused import

4b5c8ec

failing test

fcd8c07

dallan-keylogic added 2 commits October 24, 2025 15:23

additional coverage

3a446eb

expressions no hints

046ab20

bpaul4 reviewed Oct 24, 2025

View reviewed changes

MarcusHolly reviewed Oct 27, 2025

View reviewed changes

dallan-keylogic added 2 commits October 27, 2025 10:25

respond to reviewer comments

e303b5a

more reviewer comments

da60bd4

dallan-keylogic requested review from MarcusHolly and bpaul4 October 27, 2025 15:53

dallan-keylogic merged commit bb36afc into IDAES:scaling_toolbox Oct 27, 2025
28 checks passed

dallan-keylogic deleted the scale_var_from_def_con branch October 27, 2025 17:18

		f"Variable {variable} is indexed. Call with VarData "
		"children instead."

Conversation

dallan-keylogic commented Oct 24, 2025

Summary/Motivation:

Legal Acknowledgement

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

bpaul4 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 24, 2025 •

edited

Loading