Skip to content

Conversation

jasonmbray-p66
Copy link
Contributor

Fixes

Summary/Motivation:

When the Jacobian is calculated in several of the DiagnosticsToolbox methods, the optional scaled argument in idaes.core.util.scaling.get_jacobian (which actually defaults to True) is set to False every time it is called. Because it is hard-coded into the class, there is no option for the user to change this choice. One of the purposes of the numerical diagnostics is to identify scaling issues, but the scaled=False argument whenever the Jacobian is calculated means that the numerical diagnostics always report the same results every time they run, no matter what scaling the user has added to the model to address the problems identified. Once any scaling is added to the model, they are flying blind as to whether the scaling factors are working or if other variables and constraints need additional scaling.

Changes proposed in this PR:

  • Make a new config argument called use_scaled_jacobian that the user can set when creating the DiagnosticsToolbox object. This option is used throughout the class whenever the Jacobian needs to be calculated.
  • Add an optional use_scaled_jacobian argument to several class methods that are often called directly by the user, giving the user flexibility about which way to calculate the Jacobian without having to re-create the DiagnosticsToolbox object.
  • Store the Jacobian as a property in the DiagnosticsToolbox object and retrieve the stored value rather than re-calculating it every time it is needed. This saves a lot of time when running several diagnostic functions back to back, while currently the exact same Jacobian is recalculated over and over.
  • Add an optional force_jacobian_recalc argument to several class methods where the stored Jacobian is used by default if present. There could be a situation where the model is re-run with new values, making the stored Jacobian outdated. The user needs to be able to force these methods to recalculate the Jacobian based on the updated model. This argument gives the user control over when that happens.

Legal Acknowledgement

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

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  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.

@dallan-keylogic
Copy link
Contributor

The original intention was for the user to perform a Pyomo scaling transformation, then to create the diagnostic toolbox with the transformed model. That came with two downsides

  1. The Pyomo scaling transformation is slow
  2. Most initialization routines fail when called on the transformed model and will not use the scaling factors when called on the original model.

However, a solution is on the way. Pyomo's method to scale the model automatically when a .nl file is written is finally going to be fixed.. Once that is finished,

solver = get_solver(
    solver="ipopt_v2",
    solver_options={"max_iter": 50},
    writer_config={
        "scale_model": True,
        "linear_presolve": False,
    }
)

will allow the scaling factors to be used at the solver-write time. (The linear presolve is turned off because, as of this moment, the choice of pivots to determine which variables to eliminate does not take into account scaling factors. That is going to be fixed soon, but it's not clear it will make the August Pyomo release). We can then work on updating the IDAES default configuration to use the writer-time scaling. (Theoretically, we should be able to just change the option and everything will solve.)

The plan for the Diagnostics Toolbox, then, will be to have it only report scaled versions of the variables and Jacobian, because those are what IPOPT actually sees.

@jasonmbray-p66
Copy link
Contributor Author

Thanks for the background and context of why the code looks the way it does today! As you know, I was not able to follow the "typical" procedure of transforming the model because of your 2nd item - it broke the initialization routines needed for my model. The improvements to Pyomo's nl writing functionality so that scaling can be used in a more straightforward way sound great.

But how does that all relate to this PR and the question of improving the Diagnostics Toolbox? Is there something keeping you from updating the Diagnostics Toolbox to used scaled variables and Jacobians now, or are you suggesting this PR will become obsolete and therefore shouldn't be implemented?

I see no downside in implementing these proposed changes now because 1) I don't see any dependence on the pending Pyomo fixes that would require waiting until after they are released, 2) the user still has full control over using the scaled or unscaled Jacobian for diagnostics so can continue using the tools as they are now, and 3) if the default behavior changes to using the scaled Jacobian in calls to idaes.core.util.scaling.get_jacobian(), it should still work fine in the alternate cases when there are no scaling factors or when a model has been transformed following the current recommended practice.

It sounds like your plan is to go through the Diagnostics Toolbox at some point after Pyomo is patched and switch all the scaled=False options to scaled=True. Initially, that is all I planned to change as well, but I realized that with minimal added effort it could be far more useful to give the user control over this behavior. Having the defaults set in a manner that is consistent with what your recommended workflow would be is great, but I guarantee that someone somewhere will have a use for running this the opposite way and will appreciate being able to switch the assumption (I can already think of one reason). I think leaving the choice of scaled vs unscaled hard-coded inside the Diagnostics Toolbox will only lead to more confusion and problems down the road, regardless of which default behavior you choose.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants