Diagnostic tool to look for potential evaluation errors #1268

michaelbynum · 2023-09-12T16:24:09Z

Summary/Motivation:

This PR adds a method to the DiagnosticToolBox that looks for potential evaluation errors such as the log of a negative number. I'm marking it as draft while I work on tests/docs, but I wanted to create the PR to get some initial feedback.

Example:

import idaes
from idaes.core.util.model_diagnostics import DiagnosticsToolbox
import pyomo.environ as pe


m = pe.ConcreteModel()
m.x = pe.Var()
m.y = pe.Var()
m.obj = pe.Objective(expr=m.x**2 + m.y**2)
m.c1 = pe.Constraint(expr=m.y >= pe.log(m.x))
m.c2 = pe.Constraint(expr=m.y >= (m.x - 1)**2.5)
dtb = DiagnosticsToolbox(m)
dtb.report_potential_evaluation_errors()

gives

====================================================================================
Found 2 potential evaluation errors

    c1: Potential log of a negative number in log(x); Argument bounds are (-inf, inf)
    c2: Potential evaluation error in (x - 1)**2.5; base bounds are (-inf, inf); exponent bounds are (2.5, 2.5)

====================================================================================

Note that adding the constraint m.c3 = pe.Constraint(expr=m.x - 1 >= 0) does not change this report because there could still be evaluation errors in infeasible path solvers (like Ipopt). However, if this constraint is added and FBBT is used before checking for evaluation errors, then no warnings will be issued.

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.

andrewlee94 · 2023-09-12T17:36:09Z

@michaelbynum Just to put down in writing the comments I made at the working group meeting, suggest renaming report_potential_evaluation_errors to display_potential_evaluation_errors to be more consistent with our naming elsewhere, and then adding a check in report_structural_issues to collect the potential evaluation errors and report the number of issues found in the summary (if greater than zero).

Robbybp

Thanks for putting this together, @michaelbynum. A few questions on the detection of exponent evaluation errors. Also, do you think that the _EvalErrorWalker should be part of, say, pyomo.util? I guess this PR does not preclude the possibility of "promoting" it eventually if there is a need or use case.

idaes/core/util/model_diagnostics.py

michaelbynum · 2023-10-30T18:07:14Z

@michaelbynum Just to put down in writing the comments I made at the working group meeting, suggest renaming report_potential_evaluation_errors to display_potential_evaluation_errors to be more consistent with our naming elsewhere, and then adding a check in report_structural_issues to collect the potential evaluation errors and report the number of issues found in the summary (if greater than zero).

Done.

michaelbynum · 2023-10-30T18:11:08Z

Working on tests now.

Robbybp · 2023-10-30T19:48:12Z

Would it be possible or make sense to include an option where bounds are treated as strict inequalities? I.e. I think some modelers would think that log(x); x >= 0 is acceptable if their solver is (strictly) feasible path with respect to this bound.

michaelbynum · 2023-10-30T19:50:40Z

Would it be possible or make sense to include an option where bounds are treated as strict inequalities? I.e. I think some modelers would think that log(x); x >= 0 is acceptable if their solver is (strictly) feasible path with respect to this bound.

Good point. I agree. Should it be an option, or should that just be the behavior?

michaelbynum · 2023-10-30T19:51:10Z

I guess it is solver dependent, so it should probably be an option.

codecov · 2023-10-30T20:29:37Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (210db70) 77.30% compared to head (a8c0e0e) 77.33%.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1268      +/-   ##
==========================================
+ Coverage   77.30%   77.33%   +0.02%     
==========================================
  Files         390      390              
  Lines       63249    63367     +118     
  Branches    11625    11648      +23     
==========================================
+ Hits        48893    49003     +110     
- Misses      11837    11843       +6     
- Partials     2519     2521       +2

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

andrewlee94

A couple of (hopefully) small requests from me. There are a few ways we could address the first comment, and we might want to discuss different options to work out the best way to do this.

andrewlee94 · 2023-10-30T20:30:31Z

idaes/core/util/model_diagnostics.py

@@ -1229,6 +1244,10 @@ def report_structural_issues(self, stream=None):
        warnings, next_steps = self._collect_structural_warnings()
        cautions = self._collect_structural_cautions()

+        eval_error_warnings, eval_error_cautions = self._collect_potential_eval_errors()


Is there an easy way this could be moved inside the _collect_structural_warnings and _collect_structural_cautions methods? It looks like you did this for efficiency so you only need to walk the model once. However, there is an assert_no_structural_issues method as well (that only checks for warnings) which could not include this as written (although the solution might be to just add this code there as well). I am also working on adding diagnostics to the convergence tester (parameter sweep) tool, and I am looking at using these methods there too (which would mean they need to become public).

Could you add an entry to next_steps as well pointing users to the associated display method.

I think I have addressed both of these now. I realized I was being silly with the cautions. They were completely unnecessary/wrong.

michaelbynum · 2023-10-31T13:40:49Z

@Robbybp, I added an option as you suggested.

michaelbynum · 2023-10-31T13:41:24Z

If tests pass, then this will hopefully be ready to go.

Robbybp · 2023-10-31T18:21:00Z

idaes/core/util/model_diagnostics.py

+CONFIG.declare(
+    "strict_evaluation_error_detection",
+    ConfigValue(
+        default=True,
+        domain=bool,
+        description="If False, warnings will not be generated for things like log(x) with x >= 0",
+    ),
+)


I'm not sure what the default should be here. For Ipopt, with default options (i.e. bound_relax_factor=1e-8), we are strictly feasible path with respect to bounds of zero, but not other bounds (at least according to this option's documentation). So something like log(1 - x); x <= 1 could still hit the evaluation error. I think I lean towards treating this as an advanced option that people can turn on if they know what they're doing, although I think we will have to test this in the wild to really know what the "right" default is.

I tested the example you mentioned with x initialized to 2, and I did not get an evaluation error (if I set the nlp_scaling_method to none).

Which is consistent with the way we do fraction to the boundary in parapint.

That said, I agree with your overall point.

I tested the example you mentioned with x initialized to 2, and I did not get an evaluation error (if I set the nlp_scaling_method to none).

What if you set bound_push to zero (or anything less than 1*bound_relax_factor, I think)?

The point is just that the evaluation error could happen.

Yes, I'm suggesting renaming the option, although I'd prefer to get another opinion, as naming preferences can be fairly idiosyncratic.

I would be fine with a name that describes the "region of evaluation error", but would prefer to call it something other than "strict" as this makes me think of how the bounds are handled (unless others think the name makes sense).

Personally I read "strict_evaluation_error_detection" in the sense that @michaelbynum used it, but equally when I read the doc string I asked "what exactly happens when it is set to False and why?" This does have me leaning towards @Robbybp's suggestion as would focus the documentation on "why you might want to set this to relax the definition of bounds".

Since this is a potentially niche use case, maybe we should defer including this option for now (and keep the default behavior of treating bounds as if they are non-strict), then add this later if we find that the output is often dominated by bounds of zero that we don't actually expect to hit.

@andrewlee94, are you okay with removing the option for now and adding later?

That is fine by me.

ksbeattie · 2023-11-09T19:26:06Z

@Robbybp & @michaelbynum: ping. We're looking at cutting the RC just after Thanksgiving, will this make it?

michaelbynum · 2023-11-13T15:08:29Z

@Robbybp & @michaelbynum: ping. We're looking at cutting the RC just after Thanksgiving, will this make it?

Should make it.

andrewlee94 · 2023-11-13T15:18:41Z

@michaelbynum Note that you have a few lingering pylint issues too:

idaes/core/util/model_diagnostics.py:59:0: E0611: No name 'generate_standard_repn' in module 'pyomo.repn.standard_repn' (no-name-in-module)
idaes/core/util/model_diagnostics.py:1337:8: W0612: Unused variable 'res' (unused-variable)
idaes/core/util/model_diagnostics.py:1358:4: C0116: Missing function or method docstring (missing-function-docstring)
idaes/core/util/model_diagnostics.py:1840:4: C0116: Missing function or method docstring (missing-function-docstring)
idaes/core/util/model_diagnostics.py:25:0: W0611: Unused Sequence imported from typing (unused-import)
idaes/core/util/model_diagnostics.py:32:0: W0611: Unused is_fixed imported from pyomo.environ (unused-import)

michaelbynum · 2023-11-14T18:46:40Z

idaes/core/util/model_diagnostics.py

@@ -251,6 +267,14 @@ def svd_sparse(jacobian, number_singular_values):
        description="Tolerance for raising a warning for small Jacobian values.",
    ),
 )
+CONFIG.declare(
+    "warn_for_evaluation_error_at_bounds",


@Robbybp, @andrewlee94 - what do you think of this name?

Sounds good to me.

Looks good to me

Awesome. Thank you both.

michaelbynum · 2023-11-14T21:22:18Z

I'm not sure why tests are getting skipped...

andrewlee94 · 2023-11-14T21:25:03Z

@michaelbynum I think it is OK - it is just GitHub getting confused.

diagnostic tool to look for potential evaluation errors

80fabcc

andrewlee94 mentioned this pull request Sep 12, 2023

Model Diagnostics Checklist #1222

Open

31 tasks

ksbeattie added the Priority:Normal Normal Priority Issue or PR label Sep 14, 2023

ksbeattie assigned michaelbynum Sep 14, 2023

Robbybp reviewed Oct 20, 2023

View reviewed changes

idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved

idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved

idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved

idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved

michaelbynum added 3 commits October 30, 2023 11:29

merge main into eval_error_detection

368b063

working on diagnostic tool to detect possible evaluation errors

8fb3b33

working on diagnostic tool to detect possible evaluation errors

8b52c20

michaelbynum added 4 commits October 30, 2023 12:14

run black

048dfd3

working on tests for evaluation error detection

09bc21b

working on tests for evaluation error detection

6f3cae0

run black

2a135a1

michaelbynum marked this pull request as ready for review October 30, 2023 19:31

michaelbynum requested review from andrewlee94, eslickj and lbianchi-lbl as code owners October 30, 2023 19:31

andrewlee94 reviewed Oct 30, 2023

View reviewed changes

michaelbynum added 2 commits October 31, 2023 06:48

evaluation error detection

e9e8fc8

evaluation error detection

246b92a

Robbybp reviewed Oct 31, 2023

View reviewed changes

andrewlee94 approved these changes Nov 1, 2023

View reviewed changes

Merge branch 'main' into eval_error_detection

2c5b36b

michaelbynum added 2 commits November 14, 2023 11:38

fix pylint issues

df094c5

run black

261b29e

michaelbynum commented Nov 14, 2023

View reviewed changes

fix pylint issues

6a1b57e

andrewlee94 approved these changes Nov 14, 2023

View reviewed changes

fix tests

a8c0e0e

Robbybp approved these changes Nov 14, 2023

View reviewed changes

andrewlee94 merged commit d2f24ad into IDAES:main Nov 15, 2023
40 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diagnostic tool to look for potential evaluation errors #1268

Diagnostic tool to look for potential evaluation errors #1268

michaelbynum commented Sep 12, 2023

andrewlee94 commented Sep 12, 2023

Robbybp left a comment

michaelbynum commented Oct 30, 2023

michaelbynum commented Oct 30, 2023

Robbybp commented Oct 30, 2023

michaelbynum commented Oct 30, 2023

michaelbynum commented Oct 30, 2023

codecov bot commented Oct 30, 2023 •

edited

Loading

andrewlee94 left a comment

andrewlee94 Oct 30, 2023

michaelbynum Oct 31, 2023

michaelbynum commented Oct 31, 2023

michaelbynum commented Oct 31, 2023

Robbybp Oct 31, 2023

michaelbynum Oct 31, 2023

michaelbynum Oct 31, 2023

michaelbynum Oct 31, 2023

Robbybp Oct 31, 2023

Robbybp Nov 1, 2023

andrewlee94 Nov 1, 2023

Robbybp Nov 1, 2023

michaelbynum Nov 13, 2023

andrewlee94 Nov 13, 2023

ksbeattie commented Nov 9, 2023 •

edited

Loading

michaelbynum commented Nov 13, 2023

andrewlee94 commented Nov 13, 2023

michaelbynum Nov 14, 2023

andrewlee94 Nov 14, 2023

Robbybp Nov 14, 2023

michaelbynum Nov 14, 2023

michaelbynum commented Nov 14, 2023

andrewlee94 commented Nov 14, 2023

Diagnostic tool to look for potential evaluation errors #1268

Diagnostic tool to look for potential evaluation errors #1268

Conversation

michaelbynum commented Sep 12, 2023

Summary/Motivation:

Legal Acknowledgement

andrewlee94 commented Sep 12, 2023

Robbybp left a comment

Choose a reason for hiding this comment

michaelbynum commented Oct 30, 2023

michaelbynum commented Oct 30, 2023

Robbybp commented Oct 30, 2023

michaelbynum commented Oct 30, 2023

michaelbynum commented Oct 30, 2023

codecov bot commented Oct 30, 2023 • edited Loading

Codecov Report

andrewlee94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbynum commented Oct 31, 2023

michaelbynum commented Oct 31, 2023

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

Choose a reason for hiding this comment

ksbeattie commented Nov 9, 2023 • edited Loading

michaelbynum commented Nov 13, 2023

andrewlee94 commented Nov 13, 2023

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

michaelbynum commented Nov 14, 2023

andrewlee94 commented Nov 14, 2023

codecov bot commented Oct 30, 2023 •

edited

Loading

ksbeattie commented Nov 9, 2023 •

edited

Loading