Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More efficient method for check_parallel_jacobian #1395

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open
Changes from 2 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
62a6346
implement new method
dallan-keylogic Apr 10, 2024
6a46229
run black
dallan-keylogic Apr 10, 2024
5e4da39
Nonstrict zip
dallan-keylogic Apr 11, 2024
8fbb339
only relative criterion, filter out zero norm vectors
dallan-keylogic Apr 11, 2024
da664b0
Correct preallocation
dallan-keylogic Apr 11, 2024
9b8a25d
run Black
dallan-keylogic Apr 11, 2024
7fe9c5e
efficiency
dallan-keylogic Apr 11, 2024
ab30049
excessive optimization
dallan-keylogic Apr 11, 2024
8c9959c
add optional jacobian and nlp arguments plus fix test
dallan-keylogic Apr 11, 2024
5021f29
Merge branch 'main' into parallel_constraints
dallan-keylogic Apr 11, 2024
e4ce707
run black
dallan-keylogic Apr 11, 2024
8438d09
fix failing test and add additional comments
dallan-keylogic Apr 12, 2024
e048a22
Get rid of old method, try to make code more readable
dallan-keylogic Apr 19, 2024
6d53c8d
Merge branch 'main' into parallel_constraints
dallan-keylogic Apr 19, 2024
12109f9
Merge branch 'main' into parallel_constraints
dallan-keylogic May 3, 2024
e175405
merge
dallan-keylogic May 3, 2024
8cedc01
failures
dallan-keylogic May 3, 2024
a8da700
Merge branch 'main' into parallel_constraints
dallan-keylogic Jun 14, 2024
345a813
Begun changes to address issues
dallan-keylogic Jun 15, 2024
330953f
finish cleanup
dallan-keylogic Jun 17, 2024
5939198
Merge branch 'main' into parallel_constraints
dallan-keylogic Jun 17, 2024
e30c6d5
commit black
dallan-keylogic Jun 17, 2024
50903cc
pylint
dallan-keylogic Jun 17, 2024
a54f189
Begun dealing with negative phase flows
dallan-keylogic Jun 18, 2024
9bd6ce8
run Black
dallan-keylogic Jun 18, 2024
0cd2d2d
Reversion
dallan-keylogic Jun 18, 2024
42e00a2
after me, the deluge
dallan-keylogic Jun 18, 2024
940d253
reversion
dallan-keylogic Jun 18, 2024
c922565
andrew's changes
dallan-keylogic Jun 18, 2024
fb1bb91
readd attribution
dallan-keylogic Jun 20, 2024
e226647
merge
dallan-keylogic Jun 20, 2024
1aed06f
merge in complementarity, leave memorials to its sins
dallan-keylogic Jun 20, 2024
02db37e
tighten tolerance
dallan-keylogic Jun 20, 2024
631e7c5
more feedback from Robby
dallan-keylogic Jun 28, 2024
bf5f26d
Merge branch 'main' into parallel_constraints
dallan-keylogic Jun 28, 2024
ffd0071
Merge branch 'main' into parallel_constraints
Robbybp Oct 25, 2024
10250a5
add scaled option to some methods
Robbybp Oct 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion idaes/core/util/model_diagnostics.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import numpy as np
from scipy.linalg import svd
from scipy.sparse.linalg import svds, norm
from scipy.sparse import issparse, find
from scipy.sparse import issparse, find, triu

from pyomo.environ import (
Binary,
Expand Down Expand Up @@ -3653,6 +3653,72 @@ def check_parallel_jacobian(model, tolerance: float = 1e-4, direction: str = "ro

jac, nlp = get_jacobian(model, scaled=False)

# Get vectors that we will check, and the Pyomo components
# they correspond to.
if direction == "row":
components = nlp.get_pyomo_constraints()
mat = jac.tocsr()

elif direction == "column":
components = nlp.get_pyomo_variables()
mat = jac.transpose().tocsr()

norms = [norm(mat[i, :], ord="fro") for i in range(len(components))]

# Take product of all rows/columns with all rows/columns by taking outer
# product of matrix with itself
outer = mat @ mat.transpose()
Copy link
Member

Choose a reason for hiding this comment

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

The previous implementation was designed to avoid taking every possible dot product by first sorting vectors by their nonzero structure. I would have expected this to be slow due to unnecessary dot products, but in some testing, it doesn't seem slower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is sparse matrix multiplication carried out by Scipy. The key here is that 1) Sorting through which products to take should be carried out in the sparse matrix multiplication routine, not manually and
2) The looping required to do this sorting and multiplication is done in compiled C++ code, not Python code, and (should) be much faster than doing it in Python code.

Copy link
Member

Choose a reason for hiding this comment

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

The scipy matrix product is taking every possible combination of non-zero dot products, while the previous implementation takes only dot products between vectors with the same sparsity structure. This is fewer dot products, although, in practice, probably not by a wide margin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid I don't entirely follow. Scipy's matrix multiply, as near as I can tell, does the same thing. It implements the SMMP algorithm which occurs in two steps: the first to determine the nonzero structure of the matrix, the second to actually compute the values (as near as I can tell). That's basically what you're doing, except 1) it isn't filtering out entries as being "too small to contribute" and 2) it's doing it in C++ instead of Python, which is much faster.

Copy link
Member

Choose a reason for hiding this comment

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

For example, if two rows are (1, 2, 3) and (1, 2, 0), they cannot possibly be parallel. The previous implementation will not compute this dot product, while the new implementation will.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you're ruling out rows being parallel based on one having an element for the third index (3) and the other having zero for the third index? That can fail if the first row was (1, 2, 3e-8) instead of (1, 2, 3): (1, 2, 3e-8) is still effectively parallel to (1, 2, 0) even if they structurally differ.

Copy link
Member

Choose a reason for hiding this comment

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

We do this after filtering out small entries.

# Get rid of duplicate values by only taking upper triangular part of
# resulting matrix
upper_tri = triu(outer)
# List to store pairs of parallel components
parallel = []

for row, col, val in zip(*find(upper_tri), strict=True):
Copy link
Member

Choose a reason for hiding this comment

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

When I try to test this, I get an error due to this keyword argument passed to zip. I assume strict should be passed to triu, but I'm not sure exactly what else is going on here.

Copy link
Contributor Author

@dallan-keylogic dallan-keylogic Apr 11, 2024

Choose a reason for hiding this comment

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

Sorry about that. No, strict was an argument for zip. It throws an error if the iterators passed to zip are not the same length. However, it appears it was only added in Python 3.10, so I can't use it.

For reference find returns a tuple of three arrays, one containing row indices, one containing column indices, and one containing matrix values for all the nonzero entries of upper_tri. * unpacks the tuple into arguments for zip. It's a neater way of writing find_tuple = find(upper_tri) and
zip(find_tuple[0], find_tuple[1], find_tuple[2]).

if row == col:
# A vector is parallel to itself
continue
diff = abs(abs(val) - norms[row] * norms[col])
if diff <= tolerance or diff <= tolerance * max(norms[row], norms[col]):
parallel.append((components[row], components[col]))

return parallel


def check_parallel_jacobian_old(model, tolerance: float = 1e-4, direction: str = "row"):
dallan-keylogic marked this conversation as resolved.
Show resolved Hide resolved
"""
Check for near-parallel rows or columns in the Jacobian.

Near-parallel rows or columns indicate a potential degeneracy in the model,
as this means that the associated constraints or variables are (near)
duplicates of each other.

This method is based on work published in:

Klotz, E., Identification, Assessment, and Correction of Ill-Conditioning and
Numerical Instability in Linear and Integer Programs, Informs 2014, pgs. 54-108
https://pubsonline.informs.org/doi/epdf/10.1287/educ.2014.0130

Args:
model: model to be analysed
tolerance: tolerance to use to determine if constraints/variables are parallel
direction: 'row' (default, constraints) or 'column' (variables)

Returns:
list of 2-tuples containing parallel Pyomo components

"""
# Thanks to Robby Parker for the sparse matrix implementation and
# significant performance improvements

if direction not in ["row", "column"]:
raise ValueError(
f"Unrecognised value for direction ({direction}). "
"Must be 'row' or 'column'."
)

jac, nlp = get_jacobian(model, scaled=False)

# Get vectors that we will check, and the Pyomo components
# they correspond to.
if direction == "row":
Expand Down
Loading