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

Reverse Cuthill-McKee and Maximum Bipartite Matching reorderings of sparse graphs #3751

Merged
merged 19 commits into from
Aug 31, 2014
Merged

Reverse Cuthill-McKee and Maximum Bipartite Matching reorderings of sparse graphs #3751

merged 19 commits into from
Aug 31, 2014

Conversation

nonhermitian
Copy link
Contributor

Added the symrcm function that takes a symmetric graph in CSR or CSC
sparse format and returns an array of row and column permutations that
reduces the overall bandwidth of the underlying matrix. This is useful for
direct and iterative sparse linear solvers.

Added the symrcm function that takes a symmetric graph in CSR or CSC
sparse format and returns an array of row and column permutations that
reduces the overall bandwidth of the underlying matrix.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1c2106a on nonhermitian:master into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 66960e0 on nonhermitian:master into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 66960e0 on nonhermitian:master into * on scipy:master*.

@argriffing
Copy link
Contributor

Looks good, as far as I can tell. This would close #3424.

@rgommers
Copy link
Member

rgommers commented Jul 5, 2014

The name isn't very descriptive, needs to be expanded. Also, this algorithm has a fairly large overlap with csgraph.breadh_first_order. Maybe that function should grow a method keyword through which this algorithm can be selected, instead of adding this as a separate function?

@rgommers
Copy link
Member

rgommers commented Jul 5, 2014

@jakevdp want to have a look at this?

include 'parameters.pxi'

def symrcm(graph, symmetric_mode=False):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Docstring format: need a one-line summary, then a blank line. The first line gets picked up in listing in html docs.

@argriffing
Copy link
Contributor

I'd be tempted to make the changes rgommers suggested into nonhermitian's PR except that the PR is on master rather than a separate branch.

@nonhermitian
Copy link
Contributor Author

I have time tomorrow to make the changes. I will also add a maximal
bipartite matching method. Although all of these methods rely on BFS,
adding all of them as optional methods would be somewhat messy in my
opinion. One can think of many different orderings based on BFS.
On Jul 8, 2014 8:19 AM, "argriffing" notifications@github.com wrote:

I'd be tempted to make the changes rgommers suggested into nonhermitian's
PR except that the PR is on master rather than a separate branch.


Reply to this email directly or view it on GitHub
#3751 (comment).

- Added more descriptive name for RCM function
- Made changes based on @rgommers suggestions.
- Added non-symmetric maximum bipartite matching function.
@nonhermitian nonhermitian changed the title Reverse Cuthill-McKee reordering of sparse graphs Reverse Cuthill-McKee and Maximum Bipartite Matching reorderings of sparse graphs Jul 8, 2014
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7e6d153 on nonhermitian:master into * on scipy:master*.

@argriffing
Copy link
Contributor

Regarding the TravisCI test failure, this would be fixed if the indices in the bipartite matching test were explicitly dtype int32? Looking at https://github.com/scipy/scipy/blob/master/scipy/sparse/csgraph/parameters.pxi the dtype of csgraph indices seems to be restricted in this way. I don't know how easy it would be or what changes would be required to allow any integer index types.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7e6d153 on nonhermitian:master into * on scipy:master*.

@nonhermitian
Copy link
Contributor Author

I have explicitly set everything to dtype=int32. However, perhaps a fused type would work here.

@argriffing
Copy link
Contributor

Yes fused types might work; these PRs/issues have some related discussion: #243 #2194

Edit: Now I think there is some subtle typing bug in the _reordering.pyx code, so I think it will need some doctoring like in that 243 PR, or rather as in the commit c89abe67ae6748 from some PR that I can't determine.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a1461b0 on nonhermitian:master into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a1461b0 on nonhermitian:master into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 818ef33 on nonhermitian:master into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ed23297 on nonhermitian:master into * on scipy:master*.

@argriffing
Copy link
Contributor

set type of row_match after multiplication

Good catch, I guess this was the problem all along. I'd suspected it but mistakenly ruled it out when I saw similar code in qutip like https://github.com/qutip/qutip/blob/master/qutip/cy/graph_utils.pyx#L234.

@nonhermitian
Copy link
Contributor Author

QuTiP doesn’t test against NumPy 1.5.1 so I had to do the old trial and error solution method to find this.

On Jul 9, 2014, at 8:36 PM, argriffing notifications@github.com wrote:

set type of row_match after multiplication

Good catch, I guess this was the problem all along. I'd suspected it but mistakenly ruled it out when I saw similar code in qutip like https://github.com/qutip/qutip/blob/master/qutip/cy/graph_utils.pyx#L234.


Reply to this email directly or view it on GitHub.

@argriffing
Copy link
Contributor

Looks OK, but I'll wait for feedback of more experienced csgraphers.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6aed2e4 on nonhermitian:master into * on scipy:master*.

@nonhermitian
Copy link
Contributor Author

I found a bug, so hold off on pulling.

- Also set length of temp_degrees array to the max node degree of the
graph.
Test to verify node reordering works properly.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling eb85936 on nonhermitian:master into * on scipy:master*.

@pv pv removed the PR label Aug 13, 2014
@ev-br
Copy link
Member

ev-br commented Aug 29, 2014

What is the status of this PR?

@pv
Copy link
Member

pv commented Aug 29, 2014

The code looks good to me --- I assume the bug mentioned by @nonhermitian above was fixed in eb85936.

One question is using ITYPE_t which is int32 --- but it seems this is used for values with maximum value of shape[0] rather than nnz.

It would actually be very interesting if this helps with gh-3831

@nonhermitian
Copy link
Contributor Author

Yes, the error in the RCM code was fixed in the last commit. As it turns out, all of the RCM tests had graphs with nodes of the same degree, so sorting wasn't being tested. In the last commit is an example that is more general, and that caught the bug.

pv added a commit that referenced this pull request Aug 31, 2014
ENH: sparse.csgraph: Reverse Cuthill-McKee and Maximum Bipartite Matching reorderings of sparse graphs
@pv pv merged commit daa7087 into scipy:master Aug 31, 2014
@pv
Copy link
Member

pv commented Aug 31, 2014

Merging, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants