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

[contrib] Refactor incidence_analysis to cache a graph instead of a matrix #2715

Merged
merged 29 commits into from
Feb 13, 2023

Conversation

Robbybp
Copy link
Contributor

@Robbybp Robbybp commented Feb 1, 2023

EDIT: Dependence of #2715, #2716, #2723, and #2727

In summary, merging #2727 will incorporate all the others, but they are broken up to be easier to review.

Summary/Motivation:

IncidenceGraphInterface accepts a model and caches the incidence matrix (in COO format) of variables and constraints. This allows calling dulmage_mendelsohn, etc. without re-constructing the matrix, but does not allow fast look-ups of "adjacent" constraints and variables, i.e. getting all constraints that contain a variable or all variables that participate in a constraint. This would require storing the incidence matrix in both CSR and CSC formats, or alternatively storing the bipartite incidence graph as an adjacency list. This PR refactors incidence_analysis to cache the bipartite incidence graph as a NetworkX Graph instead of caching a SciPy COO matrix, allowing a get_adjacent_to method that quickly looks up adjacent variables/constraints.

Changes proposed in this PR:

  • incidence_graph attribute on IncidenceGraphInterface
  • incidence_matrix attribute is now a "property" that computes the matrix from the cached graph. This means that this attribute still works, but is now an expensive operation.
  • Refactor "algorithm methods", e.g. dulmage_mendelsohn, and the underlying implementations so we do not convert the cached graph to a COO matrix, just to convert it back to a graph
  • Add get_adjacent_to method

This PR also runs Black on incidence_analysis, for good measure.

Next steps:

This PR sets the stage for several improvements I'd like to make to incidence_analysis, which will be subsequent PRs:

  1. Add a plotting method such as [WIP] Pyomo Model Plot #2251 to IncidenceGraphInterface
  2. Update the API to remove public attributes from IncidenceGraphInterface. I would also like to update the block_triangularize and get_diagonal_blocks method names and APIs, as I think the maps returned by block_triangularize are confusing, and the name get_diagonal_blocks is an incomplete misnomer.
  3. Add documentation

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  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.

@Robbybp
Copy link
Contributor Author

Robbybp commented Feb 1, 2023

win/3.9 teest failure appears unrelated

@mrmundt
Copy link
Contributor

mrmundt commented Feb 1, 2023

win/3.9 teest failure appears unrelated

It is unrelated. We're looking into this failure - it's been consistent across all new jobs run today.

@Robbybp
Copy link
Contributor Author

Robbybp commented Feb 1, 2023

@mrmundt Thanks!

Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

There are some small typos and questions that I have, but otherwise, this is looking great.

pyomo/contrib/incidence_analysis/interface.py Outdated Show resolved Hide resolved
pyomo/contrib/incidence_analysis/interface.py Outdated Show resolved Hide resolved
pyomo/contrib/incidence_analysis/interface.py Outdated Show resolved Hide resolved
pyomo/contrib/incidence_analysis/interface.py Outdated Show resolved Hide resolved
pyomo/contrib/incidence_analysis/interface.py Outdated Show resolved Hide resolved
@mrmundt
Copy link
Contributor

mrmundt commented Feb 10, 2023

@Robbybp - There were some immense issues with our Jenkins server over the last 16ish hours. I'm hopeful that they are resolved now. Please ignore previous failures.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Base: 87.03% // Head: 86.98% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (6f1995f) compared to base (8fe21bb).
Patch coverage: 96.62% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2715      +/-   ##
==========================================
- Coverage   87.03%   86.98%   -0.06%     
==========================================
  Files         771      771              
  Lines       86162    86246      +84     
==========================================
+ Hits        74993    75019      +26     
- Misses      11169    11227      +58     
Flag Coverage Δ
linux 84.14% <96.62%> (+<0.01%) ⬆️
osx 73.61% <96.62%> (+0.02%) ⬆️
other 84.33% <96.62%> (+0.01%) ⬆️
win 81.52% <96.62%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyomo/contrib/incidence_analysis/connected.py 100.00% <ø> (ø)
...o/contrib/incidence_analysis/dulmage_mendelsohn.py 84.61% <ø> (ø)
pyomo/contrib/incidence_analysis/util.py 100.00% <ø> (ø)
pyomo/contrib/incidence_analysis/matching.py 90.00% <88.00%> (-10.00%) ⬇️
pyomo/contrib/incidence_analysis/interface.py 98.33% <97.50%> (+0.04%) ⬆️
pyomo/contrib/incidence_analysis/__init__.py 100.00% <100.00%> (ø)
...ib/incidence_analysis/common/dulmage_mendelsohn.py 100.00% <100.00%> (ø)
pyomo/contrib/incidence_analysis/triangularize.py 100.00% <100.00%> (ø)
pyomo/util/subsystems.py 89.13% <0.00%> (-8.70%) ⬇️
...ontrib/appsi/utils/collect_vars_and_named_exprs.py 93.75% <0.00%> (-6.25%) ⬇️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@blnicho blnicho merged commit 48cafd2 into Pyomo:main Feb 13, 2023
@Robbybp Robbybp deleted the incidence-graph branch February 13, 2023 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants