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

Julia 1.9.0 doesn't drop zeros when assigning to a sparse array only when the elements are Big #389

Open
LilithHafner opened this issue May 13, 2023 · 22 comments

Comments

@LilithHafner
Copy link
Member

MWE:

julia> A = spzeros(BigInt, 3, 3); A[1] = 0; A
3×3 SparseMatrixCSC{BigInt, Int64} with 1 stored entry:
 0    
     
     

This change was introduced in #296.

Originally posted by @YingboMa in JuliaLang/julia#49766

@LilithHafner
Copy link
Member Author

This is to preserve egality when possible, an admittedly weak justification in this case

julia> A = spzeros(BigInt, 3, 3); x = big(0); A[1] = x;

julia> A[1] === x
true

julia> A[2] === x
false

@YingboMa, Did you report this issue as a curiosity, because it is causing measurable performance problems, or for some other reason?

@YingboMa
Copy link

YingboMa commented May 13, 2023

It broke MTK.

@LilithHafner
Copy link
Member Author

Do you have a MWE of something that broke there?

@YingboMa
Copy link

Does the MWE alone not qualify as a bug? What's the use case for egality other than a theoretically nice sounding property?

@LilithHafner
Copy link
Member Author

The egality property makes reinterpret safe which allows us to fix #289. reinterpret only comes up in the non-isbitstype case.

Other folks might view it differently, but I think the example in the OP is ambiguous. Taken alone, I would describe it as odd but harmless. Much like this example (related to #294) from 1.8 which, alone, I would also describe as odd but harmless

julia> x = spzeros(BigFloat, 3)
3-element SparseVector{BigFloat, Int64} with 0 stored entries

julia> x[1] = -0.0
-0.0

julia> isless(x[1], 0)
false

julia> isless(-0.0, 0)
true

An example showing how the behavior in the OP breaks something (e.g. MTK) could certainly shift my thinking.

@Keno
Copy link
Contributor

Keno commented May 13, 2023

I think #390 is the real problem here. I don't think MTK would have noticed was it not for that.

@rayegun
Copy link
Member

rayegun commented May 13, 2023

Is the iszero check happening before promotion? 🤔

@LilithHafner
Copy link
Member Author

LilithHafner commented May 13, 2023

I think #390 is the real problem here.

Fortunately, that's due to a silly and fixed mistake.

Is the iszero check happening before promotion? 🤔

No, it's big(0) !== big(0).

@rayegun
Copy link
Member

rayegun commented May 13, 2023

Oh because they're objects of course... What's the reasoning for egality instead of equality?

@rayegun
Copy link
Member

rayegun commented May 13, 2023

If it's for 0.0 == -0.0 it seems to me that we should have _iszero(x) = iszero(x); _iszero(x::AbstractFloat) = ...

@LilithHafner
Copy link
Member Author

It's also for dual numbers from forward diff (JuliaDiff/ForwardDiff.jl#542) and possibly other types with nontrivial zeros. The idea is that a sparse array is an optimization which should have different performance characteristics than Array but identical semantics, so if we can't recreate the passed zero exactly, we should store it.

@rayegun
Copy link
Member

rayegun commented May 13, 2023

We can have it be the more conservative version, but allow for the overload?

_iszero(x) = x === zero(typeof(x)) Then we can support BigInt and other types? Insertion is still pretty expensive and should be avoided if possible, since we don't have pending insertions here.

@LilithHafner
Copy link
Member Author

Do you have an example use-case where making _iszero less conservative results in measurable performance gains? I have a hard time coming up with example use cases for sparse arrays that use setindex! a whole bunch of times and are not already woefully inefficient.

@YingboMa
Copy link

YingboMa commented Sep 20, 2023

Isn't it obvious? For example:

julia> using SparseArrays, LinearAlgebra, BenchmarkTools

julia> N = 500; A = diagm(0=>rand(BigFloat, N), 1=>rand(BigFloat, N-1));

julia> S = spzeros(BigFloat, N, N); S .= A; S[end, 1] = S[1, end] = 10; b = rand(N);

julia> @btime $S*$b;
  15.579 ms (1000003 allocations: 49.60 MiB)

julia> @btime $(dropzeros(S))*$b;
  102.375 μs (4007 allocations: 207.49 KiB)

julia> S
500×500 SparseMatrixCSC{BigFloat, Int64} with 250000 stored entries:
⎡⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎤
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎢⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎥
⎣⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⎦

but if we use Float64, S is actually sparse

julia> S = spzeros(Float64, N, N); S .= A; S[end, 1] = S[1, end] = 10; b = rand(N);

julia> S
500×500 SparseMatrixCSC{Float64, Int64} with 1001 stored entries:
⎡⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⎤
⎢⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⠀⠀⎥
⎢⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⡀⠀⎥
⎣⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠙⢦⎦

_iszero(x) = x === zero(typeof(x)) makes generic programming extremely hard.

@SobhanMP
Copy link
Member

SobhanMP commented Sep 20, 2023

Maybe it sense to make an is_reconstructible_zero? The default implementation could be x === zero(typeof(x)), and then it could be overwritten for different types on an as-needed basis? I think for compatibility, this should go to Julia directly.

@LilithHafner
Copy link
Member Author

LilithHafner commented Sep 20, 2023

I don't find that example particularly convincing because both the slow version and the proposed fix have a significant performance problem (using diagm instead of spdiagm).

julia> function slow(N)
           A = diagm(0=>rand(BigFloat, N), 1=>rand(BigFloat, N-1));
           S = spzeros(BigFloat, N, N); S .= A; S[end, 1] = S[1, end] = 10
           b = rand(N);
           S*b
       end
slow (generic function with 1 method)

julia> function assign_only_nonzeros(N)
           A = diagm(0=>rand(BigFloat, N), 1=>rand(BigFloat, N-1));
           S = spzeros(BigFloat, N, N); S[A .!= 0] .= A[A .!= 0]; S[end, 1] = S[1, end] = 10
           b = rand(N);
           S*b
       end
assign_only_nonzeros (generic function with 1 method)

julia> function better(N)
           A = spdiagm(0=>rand(BigFloat, N), 1=>rand(BigFloat, N-1));
           S = spzeros(BigFloat, N, N); S .= A; S[end, 1] = S[1, end] = 10
           b = rand(N);
           S*b
       end
better (generic function with 1 method)

julia> @btime slow(500);
  61.185 ms (1506044 allocations: 82.46 MiB)

julia> @btime assign_only_nonzeros(500);
  3.918 ms (12048 allocations: 2.59 MiB)

julia> @btime better(500);
  286.542 μs (6051 allocations: 443.64 KiB)

@YingboMa
Copy link

YingboMa commented Sep 20, 2023

I don't find the proposed fix particularly convincing, because should people always write S[A .!= 0] .= A[A .!= 0] for generic programming? The point of the above example is not about diagm. This happens if one mixes dense and sparse computation.

@LilithHafner
Copy link
Member Author

Apologies for the confusion. The name proposed was ambiguous. I renamed that function from proposed to assign_only_nonzeros to clarify.

The assign_only_nonzeros function models the runtime of the whole process if we implement the proposed modifications to iszero. I recommend using the better approach, which is 10x faster than that proposal.

@YingboMa
Copy link

This doesn't just happen with diagm though. This happens whenever one assigns dense matrices into a sparse matrix, if the eltype is big, and the underlaying issue is precisely due to the un-generic choice of _iszero(x) = x === zero(typeof(x)).

@LilithHafner
Copy link
Member Author

In the event that an object iszero but is not egal to it's canonical zero, we have a choice: either assign for better API alignment with dense arrays, or don't assign for better performance. The tradeoff is API alignment with dense arrays vs. performance, not generic vs un-generic. I stand by the decision to choose to support the dense array API over performance in this case.

It is plausible that there exist examples of efficient code that stops being efficient after this change. If that is the case, then I would support @SobhanMP's proposal of defining is_reconstructible_zero(x) = x === zero(typeof(x)) and then special casing non-egal reconstructable zeros (such as big). If we do add such a function, though, I think it should go in SparseArrays.jl, not Base. In the uncommon event that packages choose to override this definition, they can do so in package extensions.

@YingboMa
Copy link

Yeah, this solution sounds good.

@YingboMa
Copy link

In MTK, we had to add an explicit dropzero! because of this change https://github.com/SciML/ModelingToolkit.jl/blob/05525b540820569c01d2249096dbbc5880d943d0/src/structural_transformation/bareiss.jl#L146-L148

julia> using ModelingToolkit, BenchmarkTools, SparseArrays

julia> A = BigInt.(sprand(Int, 100, 100, 0.01)) + I;

julia> @btime ModelingToolkit.bareiss!(copy($A)) # without `dropzero!`
  129.664 ms (5869620 allocations: 115.39 MiB)
(100, -469434422448344658861925147159840003015949584463799478228775210152597681412950869211899407566099105579794178943, false)

julia> @btime ModelingToolkit.bareiss!(copy($A))
  78.866 ms (3386748 allocations: 67.56 MiB)
(100, -469434422448344658861925147159840003015949584463799478228775210152597681412950869211899407566099105579794178943, false)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants