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

ENH: in scipy.sparse.linalg.norm implement spectral norm #8472

Closed
wants to merge 6 commits into from
Closed

ENH: in scipy.sparse.linalg.norm implement spectral norm #8472

wants to merge 6 commits into from

Conversation

rafalalgo
Copy link
Contributor

ENH: in scipy.sparse.linalg.norm implement spectral norm

Reference to: #7268

@c-f-h
Copy link

c-f-h commented Feb 24, 2018

Some comments here:

  1. I think the proper value for ord to use would be 2, to be in line with the numpy version of norm: https://docs.scipy.org/doc/numpy/reference/generated/numpy.linalg.norm.html
    Keeping "spec" as an alias is probaby fine, though.

  2. Why do you import eigs?

  3. Why the explicit conversion to a csc_matrix? norm should already receive a sparse matrix per spec, so svds should able to handle whatever norm throws at it.

@rafalalgo
Copy link
Contributor Author

Ok, thanks for comment! I fixed this things.

@rafalalgo
Copy link
Contributor Author

rafalalgo commented Feb 24, 2018

Ok, without csc_matrix and info about float in matrix - svds raise error

ValueError: matrix type must be 'f', 'd', 'F', or 'D'

With this it works correctly.

@rafalalgo
Copy link
Contributor Author

@c-f-h everything is ok now?

@rafalalgo
Copy link
Contributor Author

I think that it can be merge?

@tylerjereddy tylerjereddy added scipy.sparse enhancement A new feature or improvement labels Mar 9, 2018
Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

I'm certainly not an expert on this, but added a suggestion re: the duplicated code.

from scipy.sparse.linalg import svds
x = csc_matrix(x, dtype=float)
u, s, vt = svds(x, k=1)
return s[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

The spectral norm algorithm is duplicated here, right? It might be more suitable to define a function and call it for the conditions where a spectral norm is to be used, as the duplication of code could be a maintenance burden.

Copy link
Member

Choose a reason for hiding this comment

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

The 'spec' alias should be removed.

====== ============================
None Frobenius norm
'fro' Frobenius norm
'spec' Spectral norm
Copy link
Member

Choose a reason for hiding this comment

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

Don't add the 'spec' alias, the ord=2 is sufficient.

from scipy.sparse.linalg import svds
x = csc_matrix(x, dtype=float)
u, s, vt = svds(x, k=1)
return s[0]
Copy link
Member

Choose a reason for hiding this comment

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

The 'spec' alias should be removed.

if ord in (2, 'spec'):
# spectral norm
from scipy.sparse import csc_matrix
from scipy.sparse.linalg import svds
Copy link
Member

Choose a reason for hiding this comment

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

The imports should be on top of the file.

# spectral norm
from scipy.sparse import csc_matrix
from scipy.sparse.linalg import svds
x = csc_matrix(x, dtype=float)
Copy link
Member

Choose a reason for hiding this comment

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

The conversion to csc_matrix shouldn't be necessary.
Moreover, one also has to be able to compute norms of complex matrices.

@rgommers rgommers deleted the branch scipy:master January 3, 2022 16:32
@rgommers rgommers closed this Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.sparse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants