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.

Sign up for GitHub

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

Jump to bottom

get and set diagonal of coo_matrix, and related csgraph laplacian changes #3827

Merged
merged 8 commits into from
Aug 31, 2014
Merged

get and set diagonal of coo_matrix, and related csgraph laplacian changes #3827

merged 8 commits into from
Aug 31, 2014

Conversation

argriffing
Copy link
Contributor

No description provided.

@argriffing
Copy link
Contributor Author

Should close #3720.

@pv pv added the PR label Jul 24, 2014
@argriffing
Copy link
Contributor Author

Hit the 50min cap with --mode=full. The setdiag tests are @slow so the other TravisCI configurations don't help much for this PR.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling e09e3f8 on argriffing:coo-diag into 59738d7 on scipy:master.

@argriffing
Copy link
Contributor Author

By the way, removing the placeholder COO-specific setdiag test causes the base class setdiag test to be run with COO. So superficially this PR looks like it removes a unit test but it actually adds one.

@argriffing argriffing mentioned this pull request Jul 29, 2014
Closed
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 562844c on argriffing:coo-diag into 59738d7 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 545ad8f on argriffing:coo-diag into 59738d7 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 545ad8f on argriffing:coo-diag into 59738d7 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 545ad8f on argriffing:coo-diag into 59738d7 on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 545ad8f on argriffing:coo-diag into 59738d7 on scipy:master.

@argriffing
Copy link
Contributor Author

It passes pep8 but one of the configurations failed to download wheels from pypi.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling ad75fc7 on argriffing:coo-diag into 59738d7 on scipy:master.

@pv
Copy link
Member

pv commented Jul 30, 2014

Did you push to wrong branch? This PR now contains both setdiag and csgraph changes...

@argriffing
Copy link
Contributor Author

This PR now contains both setdiag and csgraph changes...

Right, the csgraph changes use setdiag.

@argriffing
Copy link
Contributor Author

Also closes #3835 by documenting the return_diag behavior.

@argriffing argriffing changed the title get and set diagonal of coo_matrix get and set diagonal of coo_matrix, and related csgraph laplacian changes Aug 4, 2014
@argriffing argriffing mentioned this pull request Aug 4, 2014
Closed
@pv pv removed the PR label Aug 13, 2014
@argriffing
Copy link
Contributor Author

The code to set the diagonal is so messy because it is required to do things like

......o........
.......o.......
........o......
...............
...............

@pv
Copy link
Member

pv commented Aug 23, 2014

The coo_matrix.setdiag and coo_matrix.diagonal look good to me.
(Didn't yet look at the csgraph stuff.)

@pv
Copy link
Member

pv commented Aug 23, 2014

@jakevdp: do you want to take a look at the laplacian changes?

pv
pv reviewed Aug 23, 2014
View reviewed changes
scipy/sparse/coo.py
# Update the internal structure.
self.row = np.concatenate((self.row[keep], new_row))
self.col = np.concatenate((self.col[keep], new_col))
self.data = np.concatenate((self.data[keep], new_data))
Copy link
Member

Choose a reason for hiding this comment

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

Needs to invalidate canonical order here: self.has_canonical_format = False (now that gh-3646 was merged)

@argriffing
Copy link
Contributor Author

@pv I just tried separating the diag code into its own PR, but the tests failed on my system for unrelated reasons which I've added as issue #3901.

@pv
Copy link
Member

pv commented Aug 29, 2014

Merged the setdiag/diagonal parts in 62ac546

pv
pv reviewed Aug 30, 2014
View reviewed changes
scipy/sparse/csgraph/_laplacian.py
""" Return the Laplacian matrix of a directed graph.

For non-symmetric graphs the out-degree is used in the computation.
def laplacian(csgraph, normed=False, return_diag=False, use_out_degree=False):
Copy link
Member

Choose a reason for hiding this comment

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

Should the default be use_out_degree=True to preserve the previous behavior claimed in the docstring?

@pv
Copy link
Member

pv commented Aug 30, 2014

Apart from the in-degree vs out-degree default, this looks ready for merge

pv added a commit that referenced this pull request Aug 31, 2014
@pv
Merge pull request #3827 from argriffing/coo-diag
aadeec8 
BUG: sparse.csgraph: laplacian fixups
@pv pv merged commit aadeec8 into scipy:master Aug 31, 2014
@pv
Copy link
Member

pv commented Aug 31, 2014

Just checked --- the current behavior matched the previous one.

@pv pv added this to the 0.15.0 milestone Aug 31, 2014
@pv pv mentioned this pull request Aug 31, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
scipy.sparse
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@argriffing @coveralls @pv @alexbrc