Skip to content

RFC: Add cond() for sparse matrices #12467

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

Merged
merged 1 commit into from
Aug 10, 2015

Conversation

mfasi
Copy link
Contributor

@mfasi mfasi commented Aug 5, 2015

This PR would add the function cond() for sparse matrices, using the algorithm described in JuliaLang/LinearAlgebra.jl#104.

@andreasnoack
Copy link
Member

Great. I'll review later today.

@tkelman tkelman added the linear algebra Linear algebra label Aug 5, 2015
@kshyatt kshyatt added the sparse Sparse arrays label Aug 7, 2015
@mfasi mfasi force-pushed the cond_and_condest branch 2 times, most recently from 4ae92c6 to f6dd68f Compare August 7, 2015 09:07
# TODO rank

# cond
function cond(A::SparseMatrixCSC, p::Real=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

cond defaulting to different norms for sparse vs dense is a little iffy.

And I think normestinv would look a little nicer without the leading and trailing lines of whitespace inside the method - the spacing within the code is fine, but right after function and right before end doesn't serve much purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkelman I agree, this bugs me as well. Still, defaulting to a non implemented feature p=2 would be rather annoying. As an alternative, we could enforce the use of both arguments when A is sparse.

You're right, those blank lines aren't that useful. I'll delete them next time I amend the commit.

Copy link
Member

Choose a reason for hiding this comment

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

What about using 2 as the default here. Eventually, we should support the 2 norm and until we do, people won't accidentally get different norm than they expect.

@mfasi mfasi force-pushed the cond_and_condest branch 3 times, most recently from 1b9a2a8 to 28a2d01 Compare August 10, 2015 12:37
@mfasi
Copy link
Contributor Author

mfasi commented Aug 10, 2015

@andreasnoack Than you, I changed what you suggested and amended the commit.

n = max(size(A)...)
F = factorize(A)
t = min(t,n)
if length(t) > 1 || t <= 0
Copy link
Member

Choose a reason for hiding this comment

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

length is not necessary here because t is restricted to be an Integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I overlooked it. Fixed and amended.

@mfasi mfasi force-pushed the cond_and_condest branch from 28a2d01 to 4ceb4ea Compare August 10, 2015 13:31
@andreasnoack
Copy link
Member

Greta. I've just confirmed that the return type is uniquely defined for Float64 and Complex{Float64} input. Thank you for the contribution.

andreasnoack added a commit that referenced this pull request Aug 10, 2015
RFC: Add cond() for sparse matrices
@andreasnoack andreasnoack merged commit 988d67b into JuliaLang:master Aug 10, 2015
@mfasi mfasi deleted the cond_and_condest branch August 10, 2015 15:07
@mfasi
Copy link
Contributor Author

mfasi commented Aug 10, 2015

Great. I think this closes JuliaLang/LinearAlgebra.jl#104.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4ceb4ea on mfasi:cond_and_condest into ** on JuliaLang:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants