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

fix vecnorm for sparse matrices #24204

Merged
merged 1 commit into from
Oct 19, 2017
Merged

fix vecnorm for sparse matrices #24204

merged 1 commit into from
Oct 19, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Oct 19, 2017

Reduced from the failures in #17507. Failure and fix demo:

julia> @test (foo = speye(4); resize!(foo.nzval, 5)[5] = NaN; vecnorm(foo) == 2.0)
Test Failed
  Expression: begin
    foo = speye(4)
    (resize!(foo.nzval, 5))[5] = NaN
    vecnorm(foo) == 2.0
end
ERROR: There was an error during testing

julia> Base.LinAlg.vecnorm(A::SparseMatrixCSC, p::Real=2) = vecnorm(view(A.nzval, 1:nnz(A)), p)

julia> @test (foo = speye(4); resize!(foo.nzval, 5)[5] = NaN; vecnorm(foo) == 2.0)
Test Passed

Best!

@Sacha0 Sacha0 added bugfix This change fixes an existing bug linear algebra Linear algebra sparse Sparse arrays labels Oct 19, 2017
Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

Great. I fear that there might be a few more of these around. Maybe we should NaN initialize memory. :trollface: Then it would be easier to find these cases.

# make certain entries in nzval beyond
# the range specified in colptr do not
# impact vecnorm of a sparse matrix
@test (foo = speye(4); resize!(foo.nzval, 5)[5] = NaN; vecnorm(foo) == 2.0)
Copy link
Member

Choose a reason for hiding this comment

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

Please use multiple lines

Copy link
Member Author

Choose a reason for hiding this comment

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

Cheers, I will extend this test to multiple lines and then merge. Thanks!

@StefanKarpinski
Copy link
Sponsor Member

Maybe we should NaN initialize memory.

This is not completely crazy for an opt-in testing mode.

@Sacha0 Sacha0 merged commit 99ef868 into JuliaLang:master Oct 19, 2017
@Sacha0 Sacha0 deleted the fixvecnorm branch October 19, 2017 17:43
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 19, 2017

This is not completely crazy for an opt-in testing mode.

👍 Such a testing mode would have made diagnosing this issue much easier. Without a NaN serendipitously popping into the relevant uninitialized data on one run, chances are I would have been much slower to pin down the failure.

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

Successfully merging this pull request may close these issues.

3 participants