IDAES / idaes-pse

The IDAES Process Systems Engineering Framework
https://idaes-pse.readthedocs.io/
Other
218 stars 235 forks source link

Add support for Greybox model DOF counitng in degrees_of_freedom function #1512

Open avdudchenko opened 3 weeks ago

avdudchenko commented 3 weeks ago

Fixes

The current degrees_of_freedom function does not properly account for GreyBoxModel as a set of constraints, resulting in an incorrect number of DOFs being presented.

Summary/Motivation:

With increased usage of GreyBoxModel (specifically Reaktoro-pse) there is a need to support DOF counting when building IDAES model with GreyBoxModels

Changes proposed in this PR:

-add the capability to add unfixed input and output vars on GreyBox model to ComponentSet produced by unfixed_variables_in_activated_equalities_set -add a counter for the number of "equalities" in gray box model in number_activated_equalities by adding number_grey_box_equalities method

Counting the number of equalities in GreyBox model: GrayBox models do not provide traditional Pyomo Constraints that bind inputs to outputs as such, we need to assume how many GrayBox model DOFs there are.

In most cases (at least I can't think of any exceptions) GreyBoxes must be 0DOF in the eyes of Pyomo/IPOPT as each output must be some sort of function of provided inputs, regardless if the internal model in the GreyBox is 0DOF, an optimization, or it exposes internal constraints to solve the problem. As such, it should be safe to assume that the number of outputs equals the number of equality constraints.

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.
codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 87.50000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 77.06%. Comparing base (5841433) to head (1bf6ff2).

Files with missing lines Patch % Lines
idaes/core/util/model_statistics.py 87.65% 7 Missing and 3 partials :warning:
idaes/core/util/model_diagnostics.py 86.66% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1512 +/- ## ======================================= Coverage 77.05% 77.06% ======================================= Files 384 384 Lines 62338 62428 +90 Branches 10222 10245 +23 ======================================= + Hits 48034 48109 +75 - Misses 11875 11887 +12 - Partials 2429 2432 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

Robbybp commented 3 weeks ago

In most cases (at least I can't think of any exceptions) GreyBoxes must be 0DOF in the eyes of Pyomo/IPOPT as each output must be some sort of function of provided inputs, regardless if the internal model in the GreyBox is 0DOF, an optimization, or it exposes internal constraints to solve the problem. As such, it should be safe to assume that the number of outputs equals the number of equality constraints.

IIRC, outputs just get translated to equality constraints of the form y = GB(x), but the greybox can define other equality constraints as well. So the number of equality constraints is at least the number of outputs, but can be greater.

avdudchenko commented 3 weeks ago

@Robbybp : Right, so you can include equality constraints in a graybox and send them to the solver (ipopt) - but the variables in those constraints are not actually in pyomo space- and thus we can't count them (hence why here I am only looking at inputs and outputs and ignoring any internal equality constraints). (e.g. looking at this example)

At this point, we just want to look at the graybox as a constraint that binds exposed pyomo inputs to outputs on it, and assume that the users properly setup the internal graybox model to have the correct DOFS... ideally we would have some metadata on the graybox that tells us its internal # of DOFs so we can include it in the total count. But I opt to avoid that for now... unless we want to add support for this now.

avdudchenko commented 3 weeks ago

Thanks for this, it is a case we have not considered and Greybox models are becoming more important. To start with, I had a few suggestions for more tests and modularization of the code.

However, a more significant request would be to add these tools and (and some test cases) into the diagnostics toolbox as well. We should at a minimum check that the DoF and model statistics output (number of variables and constraints) accurately reflect the model with a Greybox present. It might also be nice to add additional sub-rows in the model statistics for greybox components when they are present. Finally, we should look for any edge cases that might occur with the rest of the toolbox and if there are other things we need to consider when using greybox models.

So I started adding display to diagnostics tools

lbianchi-lbl commented 3 weeks ago

This is just waiting for @avdudchenko to extend the reporting functionality for diagnostics. This should be in the November release.

avdudchenko commented 3 weeks ago

Ready for another round of reviews, I updated model statistics and diagnostics to try and treat greybox models as part of full model, so variables and equities from greyboxes will be added with other stats as well, in addition to providing additional grey box model outputs via the diagnostic toolbox, which will show this:

    Activated Blocks: 1 (Deactivated: 0)
    Free Variables in Activated Constraints: 4 (External: 0)
        Free Variables with only lower bounds: 0
        Free Variables with only upper bounds: 0
        Free Variables with upper and lower bounds: 0
    Fixed Variables in Activated Constraints: 3 (External: 0)
    Activated Equality Constraints: 5 (Deactivated: 3)
    Activated Inequality Constraints: 0 (Deactivated: 0)
    Activated Objectives: 0 (Deactivated: 0)
    GreyBox Statistics
        Activated GreyBox models: 1 (Deactivated: 1)
        Activated GreyBox Equalities: 3 (Deactivated: 3)
        Free Variables in Activated GreyBox Equalities: 4 (Fixed: 1)
Robbybp commented 3 weeks ago

I'm curious what happens in the structural analysis when you have a greybox model. I can't imagine the under/overconstrained subsystems are handled correctly. Do you know what happens in this case?

dallan-keylogic commented 9 hours ago

You also should add a warning to the structural analysis tool that it's not compatible with grey box models, then make sure that report_structural_issues in the diagnostics toolbox has sensible results.

avdudchenko commented 8 hours ago

You also should add a warning to the structural analysis tool that it's not compatible with grey box models, then make sure that report_structural_issues in the diagnostics toolbox has sensible results.

oof, I have not worked with this tool set deeply enough to even know where to start.

Can we leave this for another PR. At this stage, I doubt many users would try to use diagnostics with Greybox models in general... so it should be okay. I would rather make a new PR after testing and finding all issues with diagnostic tools and then add appropriate doc/warnings.

Robbybp commented 7 hours ago

Can we leave this for another PR. At this stage, I doubt many users would try to use diagnostics with Greybox models in general... so it should be okay. I would rather make a new PR after testing and finding all issues with diagnostic tools and then add appropriate doc/warnings.

I agree, but I don't want to confidently give wrong analyses if we're supporting GreyBox in one part of the tool (model statistics). Can you raise a "beta warning" or something in report_structural_issues and report_numerical_issues if greybox blocks are detected?

Robbybp commented 7 hours ago

@avdudchenko Can you also open an issue about handling GBBs in structural and numerical diagnostics? If you provide a MWE to test on, I can try to update incidence analysis to do something reasonable.

avdudchenko commented 6 hours ago

@avdudchenko Can you also open an issue about handling GBBs in structural and numerical diagnostics? If you provide a MWE to test on, I can try to update incidence analysis to do something reasonable.

Will do.

avdudchenko commented 6 hours ago

Can we leave this for another PR. At this stage, I doubt many users would try to use diagnostics with Greybox models in general... so it should be okay. I would rather make a new PR after testing and finding all issues with diagnostic tools and then add appropriate doc/warnings.

I agree, but I don't want to confidently give wrong analyses if we're supporting GreyBox in one part of the tool (model statistics). Can you raise a "beta warning" or something in report_structural_issues and report_numerical_issues if greybox blocks are detected?

I added error raising for those functions for now.

dallan-keylogic commented 5 hours ago

The Exceptions being raised when report_structural_issues and report_numerical_issues are being called are good, but the user could still call the structural analysis tool by itself and run into issues. Why not just have a single exception when creating the DiagnosticsToolbox (and SVDToolbox and DegeneracyHunter) if the model has any grey box models? You'd have to comment out the tests on model statistics, but leave them in so we can use them later. Also add tests making sure the exception is raised when creating any of those objects.

We can sort out which methods support grey box models and which ones don't in another issue/PR and fine tune our warnings/errors later.

avdudchenko commented 5 hours ago

The Exceptions being raised when report_structural_issues and report_numerical_issues are being called are good, but the user could still call the structural analysis tool by itself and run into issues. Why not just have a single exception when creating the DiagnosticsToolbox (and SVDToolbox and DegeneracyHunter) if the model has any grey box models? You'd have to comment out the tests on model statistics, but leave them in so we can use them later. Also add tests making sure the exception is raised when creating any of those objects.

We can sort out which methods support grey box models and which ones don't in another issue/PR and fine tune our warnings/errors later.

Done, I also did not remove prior tests as they directly call on report function.