Skip to content

Comments

Infeasibility diagnostic tool#1409

Merged
andrewlee94 merged 5 commits intoIDAES:mainfrom
bknueven:infeasibility_diagnostic_tool
May 12, 2024
Merged

Infeasibility diagnostic tool#1409
andrewlee94 merged 5 commits intoIDAES:mainfrom
bknueven:infeasibility_diagnostic_tool

Conversation

@bknueven
Copy link
Contributor

@bknueven bknueven commented May 10, 2024

Fixes #1405

Summary/Motivation:

See discussion on #1405

Changes proposed in this PR:

  • Add basic wrapper for pyomo.contrib.iis.mis.compute_infeasibility_explanation to idaes.core.util.model_diagnositics (6099416)
  • Add test for wrapper (199e2b7)
  • Integrate compute_infeasibility_explanation into the DiagnosticsToolbox (a931a77)

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.

@bknueven bknueven requested review from Robbybp and andrewlee94 May 10, 2024 19:49
@bknueven
Copy link
Contributor Author

bknueven commented May 10, 2024

The spell checker is hitting false positives on "mis" and "MIS".

andrewlee94
andrewlee94 suggested changes May 10, 2024
View reviewed changes
Copy link
Contributor

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few minor requests, of which half are spell checker issues.

idaes/core/util/model_diagnostics.py Outdated
ConfigValue(
default=1e-6,
domain=float,
description="Feasiblity tolerance for idenifying infeasible constraint and bounds",
Copy link
Contributor

@andrewlee94 andrewlee94 May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "idenifying"

Copy link
Contributor

@lbianchi-lbl lbianchi-lbl May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also "feasiblty" I believe

Copy link
Contributor

@andrewlee94 andrewlee94 May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably also be constraints (plural).

Copy link
Contributor Author

@bknueven bknueven May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 34668b0 and 40670e4

idaes/core/util/model_diagnostics.py Outdated
return ill_cond


def compute_infeasibility_explanation(
Copy link
Contributor

@andrewlee94 andrewlee94 May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be done as a separate method, or could it just be written directly into the toolbox class?

Copy link
Contributor Author

@bknueven bknueven May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree -- I couldn't decide at what level things should be integrated. Implemented this suggestion in 40670e4.

idaes/core/util/model_diagnostics.py Outdated


def compute_infeasibility_explanation(
model, solver, tee=False, tolerance=1e-6, stream=None
Copy link
Contributor

@andrewlee94 andrewlee94 May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have solver=None here, and a check for:

if solver is None:
    solver = get_solver()

Copy link
Contributor Author

@bknueven bknueven May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 40670e4

idaes/core/util/model_diagnostics.py Outdated
that removing any single constraint or variable bound would result in a
feasible subsystem.

This function is a wrapper for the same capability in pyomo.contrib.iis.mis.
Copy link
Contributor

@andrewlee94 andrewlee94 May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to add a few exclusions to the spell-checker file as well.

Copy link
Contributor Author

@bknueven bknueven May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 34668b0

@andrewlee94
Copy link
Contributor

andrewlee94 commented May 10, 2024

@bknueven You can add exclusions for the spell checker here: https://github.com/IDAES/idaes-pse/blob/main/.github/workflows/typos.toml

@bknueven bknueven requested a review from ksbeattie as a code owner May 10, 2024 20:22
@andrewlee94 andrewlee94 added enhancement New feature or request Priority:Normal Normal Priority Issue or PR diagnostics labels May 10, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2024

Codecov Report

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

Project coverage is 77.88%. Comparing base (5d2f94c) to head (40670e4).

Files Patch % Lines
idaes/core/util/model_diagnostics.py 81.25% 1 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
- Coverage   77.89%   77.88%   -0.01%     
==========================================
  Files         394      394              
  Lines       65053    65069      +16     
  Branches    14383    14385       +2     
==========================================
+ Hits        50670    50680      +10     
- Misses      11793    11798       +5     
- Partials     2590     2591       +1

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

Robbybp
Robbybp approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@Robbybp Robbybp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks Ben!

@andrewlee94 andrewlee94 enabled auto-merge (squash) May 12, 2024 20:36
@andrewlee94 andrewlee94 disabled auto-merge May 12, 2024 20:36
@andrewlee94 andrewlee94 merged commit 17e9e11 into IDAES:main May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Robbybp Robbybp Robbybp approved these changes

@dallan-keylogic dallan-keylogic Awaiting requested review from dallan-keylogic dallan-keylogic is a code owner

@lbianchi-lbl lbianchi-lbl Awaiting requested review from lbianchi-lbl

@ksbeattie ksbeattie Awaiting requested review from ksbeattie

+1 more reviewer

@andrewlee94 andrewlee94 andrewlee94 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bknueven bknueven

Labels

diagnostics enhancement New feature or request Priority:Normal Normal Priority Issue or PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add wrapper for pyomo.contrib.iis.compute_infeasibility_explanation to diagnositics toolbox

5 participants

@bknueven @andrewlee94 @codecov-commenter @Robbybp @lbianchi-lbl