Add diagnostics tests for all unit models #1375

andrewlee94 · 2024-03-14T16:29:13Z

Fixes #1374

Summary/Motivation:

This PR adds tests using the diagnostics toolbox to all unit model test cases. Also adds catch for error identified in #1374

Changes proposed in this PR:

Patch/workaround for ASL bug in diagnostics toolbox
Updates tests for all core unit models to include checks for structural and numerical issues.

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.

…recated)

… into diagnostics_testing

…idaes-pse into diagnostics_testing

bpaul4

Everything looks good to me, I just had a couple of comments.

bpaul4 · 2024-03-14T17:56:46Z

idaes/models/properties/examples/saponification_thermo.py

@@ -83,15 +83,15 @@ def build(self):

        # Heat capacity of water
        self.cp_mol = Param(
-            mutable=False,
+            mutable=True,


What was the reason for making the Params mutable?

As they have units, Pyomo was implicitly making them mutable and logging a warning. I changed them to be explicitly mutable to suppress the warning.

bpaul4 · 2024-03-14T17:58:39Z

idaes/models/unit_models/tests/test_heat_exchanger.py

@@ -548,21 +556,11 @@ def test_build(self, btx):
        assert number_total_constraints(btx) == 38
        assert number_unused_variables(btx) == 0

-    @pytest.mark.integration


Does the Diagnostics check include looking for the correct units on each variable, as "test_units" looks for?

No, but I felt that was somewhat superfluous if units were consistent.

…stics_testing

Robbybp · 2024-03-15T19:27:57Z

idaes/core/util/scaling.py

+# There appears to be a bug in the ASL which causes terminal failures
+# if you try to create multiple ASL structs with different external
+# functions in the same process. This causes pytest to crash during testing.
+# To avoid this, register all known external functions before we call
+# PyNumero.
+ext_funcs = ["cubic_roots", "general_helmholtz_external", "functions"]
+library_set = set()
+libraries = []
+
+for f in ext_funcs:
+    library = find_library(f)
+    if library not in library_set:
+        library_set.add(library)
+        libraries.append(library)
+
+if "AMPLFUNC" in os.environ:
+    env_str = "\n".join([os.environ["AMPLFUNC"], *libraries])
+else:
+    env_str = "\n".join(libraries)
+
+os.environ["AMPLFUNC"] = env_str
+


Why is this in the scaling module? I could see two different approaches for implementing this workaround:

We don't attempt to fix this for users (until we get a bug report about it), and just add the workaround to a testing module somewhere.

We attempt to fix this for users.

This seems to be going with option 2. However, I would argue that, if we're trying to give users access to the workaround, we should put this in a high-level module or __init__.py. I say this because users could hit this issue in a wide variety of situations in which scaling is not imported. If we just want to narrowly fix the issue with the tests, I would put this in the test module that is triggering the bug (I believe test_model_diagnostics).

The function that is being used for the Jacobian is here, hence why I moved it from the diagnostics. I think this is the only place we use PyNumero for now as well, so it would cover many user cases. The reason I did not put it in the top level __init__.py is just because I am hesitant to have put code there (mostly learning from @lbianchi-lbl's warnings).

This could lead to very confusing behavior where a user experiences a bug only if idaes.core.util.scaling is not imported. I would make sure this is run either whenever IDAES is imported or only in the specific test file that is causing a problem. I am happy to hear specific arguments why the former is a bad idea, or to take a third opinion into consideration.

@Robbybp I agree with your point that this code being run inconsistently depending on whether idaes.core.util.scaling is imported should be avoided as it could lead to very hard to debug behaviors.

As a clarification, my take is that nonstandard code being run as import-time side effects should be avoided if possible, for the reason above and others (including there not being an easy way to "opt out" or "undo" the changes if needed). However, if the choice is between "import-time side effects that are run always" vs "import-time side effects that only run sometimes", the first is IMO preferable. Additionally, since there's already a lot happening in idaes/__init__.py/idaes/config.py, I can see the argument for keeping all this type of code in one(-ish) place.

In summary, I agree with you in that idaes/__init__.py (or any other module that's unconditionally/consistently imported) would be the most reasonable place to put this code.

idaes/core/util/scaling.py

Robbybp

One small question, but this looks good. I think this is a great step to take. (And thanks for catching the SuperLU error.)

Robbybp · 2024-03-18T17:25:38Z

idaes/models/unit_models/tests/test_heater.py

-        m.fs.unit.inlet.conc_mol_comp[0, "SodiumAcetate"].fix(0.0)
-        m.fs.unit.inlet.conc_mol_comp[0, "Ethanol"].fix(0.0)
+        m.fs.unit.inlet.conc_mol_comp[0, "SodiumAcetate"].fix(1e-8)
+        m.fs.unit.inlet.conc_mol_comp[0, "Ethanol"].fix(1e-8)


What was the warning that required this change? I assume something like a natural log of each concentration?

Value fixed at exactly the bound. Whilst that is not a "real" error in the numerical sense (as it is a fixed variable), it is poor modeling practice to have 0 concentrations (and thus zero flows) so I set it to an EPS instead.

I disagree that having 0 concentrations is poor practice. As long as at least one concentration is non-zero, we don't have a zero total flow. That said, I think this change is fine.

You don't have to deal with zero total flow, but if you either have a power-law concentration expression or calculate entropy of mixing anywhere, you end up with an evaluation error.

For my own education/curiosity, where do concentration power-law expressions show up?

Probably power-law concentration isn't the best way to describe it, but in conventional mass-action equilibrium expressions:

Edit: Probably the way to go there would be multiplying both sides by the reactants concentrations, but then you have degeneracy instead of an evaluation error.

This, and the fact that thermo properties often have logs or inverse functions in them is why I consider it bad practice to set any concentration/mass/mole fraction to absolute zero. I don't think this particular case actually has any of those, but I try to do this everywhere to err on the side of caution (and maybe get others to learn from me).

bpaul4

The updates look good to me.

codecov · 2024-03-21T20:07:31Z

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 77.62%. Comparing base (ecb07d8) to head (aef5fb1).

Files	Patch %	Lines
idaes/core/util/model_diagnostics.py	72.22%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1375      +/-   ##
==========================================
+ Coverage   77.61%   77.62%   +0.01%     
==========================================
  Files         391      391              
  Lines       64355    64375      +20     
  Branches    14251    14257       +6     
==========================================
+ Hits        49946    49970      +24     
- Misses      11832    11834       +2     
+ Partials     2577     2571       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Robbybp and others added 7 commits March 6, 2024 14:09

update imports of native_types and pyomo_constant_types (which is dep…

ed3eb3a

…recated)

Adding next batch of diagnostics tests

842650b

Next batch of daignsotics tests

6d09b5c

Merge branch 'consttype-import' of https://github.com/Robbybp/idaes-pse…

1389783

… into diagnostics_testing

Work around for ASL issue

01e010c

Adding more diagnostics checks

cc9c240

Last unit model diagnostics tests

2189347

andrewlee94 requested review from bpaul4, dallan-keylogic and lbianchi-lbl as code owners March 14, 2024 16:29

andrewlee94 self-assigned this Mar 14, 2024

andrewlee94 added Priority:Normal Normal Priority Issue or PR testing Issues dealing with testing of code unit models Issues dealing with the unit model libraries labels Mar 14, 2024

Andrew Lee added 3 commits March 14, 2024 12:30

Merge branch 'main' into diagnostics_testing

7755690

Fixing typo

8a9a7ee

Merge branch 'diagnostics_testing' of https://github.com/andrewlee94/…

92b54f4

…idaes-pse into diagnostics_testing

bpaul4 approved these changes Mar 14, 2024

View reviewed changes

Improving fix for ASL issue

1b91594

andrewlee94 requested a review from Robbybp March 15, 2024 15:45

Andrew Lee added 4 commits March 15, 2024 13:47

Better implementation of fix

7f4a976

Merge branch 'main' of https://github.com/IDAES/idaes-pse into diagno…

5af6d99

…stics_testing

Fixing pylint and Python 3.8 failures

448081a

Removing old implementation of workaround

2d8afda

andrewlee94 requested a review from bpaul4 March 15, 2024 19:10

Fixing noisy test

a4a84dd

Robbybp reviewed Mar 15, 2024

View reviewed changes

andrewlee94 requested a review from ksbeattie as a code owner March 18, 2024 14:27

Moving registration of external functions

4cf5fb2

andrewlee94 requested a review from Robbybp March 18, 2024 14:28

Andrew Lee added 2 commits March 18, 2024 11:13

Reverting to version that works on Windows

55258cf

Trying another way to get binary files

cd47ae2

Robbybp approved these changes Mar 18, 2024

View reviewed changes

bpaul4 approved these changes Mar 20, 2024

View reviewed changes

Merge branch 'main' into diagnostics_testing

aef5fb1

ksbeattie enabled auto-merge (squash) March 21, 2024 18:43

ksbeattie merged commit 8948c6c into IDAES:main Mar 21, 2024
45 checks passed

andrewlee94 deleted the diagnostics_testing branch March 21, 2024 20:07

Robbybp mentioned this pull request Mar 23, 2024

Add parallel constraint/variable check to report_numerical_issues #1385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add diagnostics tests for all unit models #1375

Add diagnostics tests for all unit models #1375

andrewlee94 commented Mar 14, 2024 •

edited

Loading

bpaul4 left a comment

bpaul4 Mar 14, 2024

andrewlee94 Mar 14, 2024

bpaul4 Mar 14, 2024

andrewlee94 Mar 14, 2024

Robbybp Mar 15, 2024

andrewlee94 Mar 15, 2024

Robbybp Mar 15, 2024

lbianchi-lbl Mar 15, 2024

Robbybp left a comment

Robbybp Mar 18, 2024

andrewlee94 Mar 18, 2024

Robbybp Mar 18, 2024

dallan-keylogic Mar 19, 2024

Robbybp Mar 19, 2024

dallan-keylogic Mar 19, 2024 •

edited

Loading

andrewlee94 Mar 20, 2024

bpaul4 left a comment

codecov bot commented Mar 21, 2024

Add diagnostics tests for all unit models #1375

Add diagnostics tests for all unit models #1375

Conversation

andrewlee94 commented Mar 14, 2024 • edited Loading

Fixes #1374

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

bpaul4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbybp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dallan-keylogic Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpaul4 left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2024

Codecov Report

andrewlee94 commented Mar 14, 2024 •

edited

Loading

dallan-keylogic Mar 19, 2024 •

edited

Loading