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

Wrong evaluation in sparse/higherorderfns.jl fix#23857 #24806

Merged
merged 5 commits into from
Mar 22, 2018
Merged

Wrong evaluation in sparse/higherorderfns.jl fix#23857 #24806

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9091782
wrong evaluation in sparse/higherorderfns.jl fix#23857
KlausC Nov 27, 2017
2988a7f
Merge branch 'master' into fix#23857
KlausC Jan 26, 2018
92de96f
Merge branch 'master' into fix#23857
KlausC Mar 1, 2018
6aaf4d6
Merge branch 'master' into fix#23857
KlausC Mar 21, 2018
fa464bf
`_broadcast_nonzeropre` fixed all branches of if-statement
KlausC Mar 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 8 additions & 16 deletions stdlib/SparseArrays/src/higherorderfns.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ function _broadcast_notzeropres!(f::Tf, fillvalue, C::SparseVecOrMat, A::SparseV
# Build dense matrix structure in C, expanding storage if necessary
_densestructure!(C)
# Populate values
fill!(storedvals(C), fillvalue)
fill!(storedvals(C), fillvalue) # value need replaced if A[i,j] != 0 or B[i,j) != 0
# Cases without vertical expansion
if numrows(A) == numrows(C)
@inbounds for (j, jo) in zip(columns(C), _densecoloffsets(C))
Expand Down Expand Up @@ -719,23 +719,15 @@ function _broadcast_notzeropres!(f::Tf, fillvalue, C::SparseVecOrMat, A::SparseV
Bk, stopBk = numcols(B) == 1 ? (colstartind(B, 1), colboundind(B, 1)) : (colstartind(B, j), colboundind(B, j))
Bx = Bk < stopBk ? storedvals(B)[Bk] : zero(eltype(B))
fzAvB = f(zero(eltype(A)), Bx)
if _iszero(fzAvB)
Copy link
Member

Choose a reason for hiding this comment

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

Having briefly refreshed my memory of these methods, I believe that the _iszero(fzAvB) check (and similar _iszero(fvAzB) check in the preceding branch where A rather than B expands vertically) should instead be something to the effect of fzAvB == fillvalue in the methods for the not-zero-preserving-f case. These branches should stay around, however, as otherwise performance may degrade appreciably. Regrettably a nanosoldier suite for these methods remains a "one day" project. Best!

while Ak < stopAk
Ai = Ak < stopAk ? storedinds(A)[Ak] : rowsentinelA
for Ci::indtype(C) in 1:numrows(C)
if Ai == Ci
Cx = f(storedvals(A)[Ak], Bx)
Cx != fillvalue && (storedvals(C)[jo + storedinds(A)[Ak]] = Cx)
Ak += oneunit(Ak)
end
else
Ai = Ak < stopAk ? storedinds(A)[Ak] : rowsentinelA
for Ci::indtype(C) in 1:numrows(C)
if Ai == Ci
Cx = f(storedvals(A)[Ak], Bx)
Ak += oneunit(Ak); Ai = Ak < stopAk ? storedinds(A)[Ak] : rowsentinelA
else
Cx = fzAvB
end
Cx != fillvalue && (storedvals(C)[jo + Ci] = Cx)
Ak += oneunit(Ak); Ai = Ak < stopAk ? storedinds(A)[Ak] : rowsentinelA
else
Cx = fzAvB
end
Cx != fillvalue && (storedvals(C)[jo + Ci] = Cx)
end
end
end
Expand Down
8 changes: 8 additions & 0 deletions stdlib/SparseArrays/test/higherorderfns.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,16 @@ end
end
end
end

# fix#23857
@test sparse([1; 0]) ./ [1] == sparse([1.0; 0.0])
@test isequal(sparse([1 2; 1 0]) ./ [1; 0], sparse([1.0 2; Inf NaN]))
@test sparse([1 0]) ./ [1] == sparse([1.0 0.0])
@test isequal(sparse([1 2; 1 0]) ./ [1 0], sparse([1.0 Inf; 1 NaN]))

end


@testset "broadcast[!] implementation capable of handling >2 (input) sparse vectors/matrices" begin
N, M, p = 10, 12, 0.3
f(x, y, z) = x + y + z + 1
Expand Down