-
Notifications
You must be signed in to change notification settings - Fork 241
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
dallan-keylogic
wants to merge
37
commits into
IDAES:main
Choose a base branch
from
dallan-keylogic:parallel_constraints
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
62a6346
implement new method
dallan-keylogic 6a46229
run black
dallan-keylogic 5e4da39
Nonstrict zip
dallan-keylogic 8fbb339
only relative criterion, filter out zero norm vectors
dallan-keylogic da664b0
Correct preallocation
dallan-keylogic 9b8a25d
run Black
dallan-keylogic 7fe9c5e
efficiency
dallan-keylogic ab30049
excessive optimization
dallan-keylogic 8c9959c
add optional jacobian and nlp arguments plus fix test
dallan-keylogic 5021f29
Merge branch 'main' into parallel_constraints
dallan-keylogic e4ce707
run black
dallan-keylogic 8438d09
fix failing test and add additional comments
dallan-keylogic e048a22
Get rid of old method, try to make code more readable
dallan-keylogic 6d53c8d
Merge branch 'main' into parallel_constraints
dallan-keylogic 12109f9
Merge branch 'main' into parallel_constraints
dallan-keylogic e175405
merge
dallan-keylogic 8cedc01
failures
dallan-keylogic a8da700
Merge branch 'main' into parallel_constraints
dallan-keylogic 345a813
Begun changes to address issues
dallan-keylogic 330953f
finish cleanup
dallan-keylogic 5939198
Merge branch 'main' into parallel_constraints
dallan-keylogic e30c6d5
commit black
dallan-keylogic 50903cc
pylint
dallan-keylogic a54f189
Begun dealing with negative phase flows
dallan-keylogic 9bd6ce8
run Black
dallan-keylogic 0cd2d2d
Reversion
dallan-keylogic 42e00a2
after me, the deluge
dallan-keylogic 940d253
reversion
dallan-keylogic c922565
andrew's changes
dallan-keylogic fb1bb91
readd attribution
dallan-keylogic e226647
merge
dallan-keylogic 1aed06f
merge in complementarity, leave memorials to its sins
dallan-keylogic 02db37e
tighten tolerance
dallan-keylogic 631e7c5
more feedback from Robby
dallan-keylogic bf5f26d
Merge branch 'main' into parallel_constraints
dallan-keylogic ffd0071
Merge branch 'main' into parallel_constraints
Robbybp 10250a5
add scaled option to some methods
Robbybp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.