[WIP] Scale named expressions in scaling transformation

[Robbybp]

Fixes part of #3344

Corrects scaled variables in named expressions, but does not use scaling factors assigned to named expressions.

Summary/Motivation:

Expressions in scaled model were incorrect as they would use scaled variables, but wouldn't have the factor necessary to un-apply the scaling in the expression itself. This normally wouldn't lead to incorrect solutions, as constraints removed named expressions when replacing variables with their scaled versions.

Changes proposed in this PR:

• Don't remove named expressions when scaling
• Don't descend into named expressions when scaling a parent expression
• Scale named expression separately

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

1. I agree my contributions are submitted under the BSD license.
2. 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.

[mrmundt]

Software-quality-wise, this is lovely. Needs another review from someone for correctness of functionality.

[jsiirola]

I think this needs a slightly more convoluted implementation to cover cases where Expression objects are not on the (sub)block that is being transformed. The expression walker needs to record all Expression objects that it encounters to make sure that they are appropriately transformed. Maybe we should remove_named_expressiosn if the Expression is out-of-scope, and transform them if they are in-scope?

Consider:

m = pyo.ConcreteModel()
m.inner_expr = pyo.Expression()

m.sub = Block()
m.sub.x = pyo.Var(initialize=10)
m.sub.y = pyo.Var(initialize=0.1)
m.sub.obj = Objective(expr=m.sub.x + m.sub.y + m.inner_expr)
m.sub.scaling_factor = pyo.Suffix(direction=pyo.Suffix.EXPORT)
m.sub.scaling_factor[m.sub.x] = 0.1
m.sub.scaling_factor[m.sub.y] = 10

m.inner_expr = 3 + m.sub.x**2 + m.sub.y

m_scaled = pyo.TransformationFactory('core.scale_model').create_using(
    m.sub, rename=False
)

[Robbybp]

Good point. We could:

1. Record the named expressions we encounter in the constraint expressions and scale these, as well as all named expressions that live in the block
2. Do as you suggest and scale in-place all named expressions that live in the block, but replace those that live outside.

I think I lean towards 2, but I don't see a huge problem with 1 (if the variables have been scaled, then they should be replaced with their scaled equivalent anywhere they appear). Regardless, this implementation will actually require some thought.

[jsiirola]

There is another case to worry about: what about a Constraint that lives outside the transformation scope that references the same Expression as a Constraint within the transformation scope? If we transform the Expression, then that will partially scale the out-of-scope Constraint.

[Robbybp]

Marking as WIP until we decide how to deal with the additional complexities John points out.

[Robbybp]

Maybe the simplest option is to just remove named Expressions from constraint bodies and also remove them from the block that was scaled. I don't see a way to protect against named expressions being used elsewhere, unless we explicitly duplicate all the named expressions used by the scaled block.

[Robbybp]

Maybe an option to opt-in to scaling Expressions in-place, for users who know that the expressions aren't being used elsewhere (which is 90% of use cases, where we just want to scale an entire self-contained model).

[jsiirola]

These issues become moot if we only allow out-of-place transformations. If we start by cloning the (sub)block that we are transforming, that will break any external references to the Expression objects. Then it will be safe to transform all the local Expression objects in the block hierarchy and substitute out any other Expression objects encountered in Constraint / Objective expressions.

[Robbybp]

My current preference is to:

• Transform Expressions within the block scope to use scaled variables.
• If there are Expressions in the constraints that live outside the block scope, throw a warning, then replace them explicitly with their (scaled) expr.

However, this will break any constraints outside our scope that use expressions that were scaled. The reason I'm suggesting this is that we already break constraints outside our scope that use variables that get scaled. Another option is to clone expressions used by the constraint that are outside the block scope (and then replacing the original expressions with the cloned, scaled versions). This would attempt to preserve the common-subexpression efficiency within the scaled block while not breaking other uses of these expressions. And yes, this all only matters if we are doing the transformation in-place.

This also begs the question of what to do with variables that participate in a constraint, but are not within the block scope. These currently will not be scaled, even if they have a scaling factor on the block.

What this is revealing to me is that in-place expression transformations are not well-defined on a non-self-contained model. We could just throw an error if we detect a non-self-contained model (variables or expressions in a constraint that don't live in the block scope).

[Robbybp]

Closing this as I'm not actively working on it. Will re-open when I get back to it.