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

Make (again) length type match index type in sparse vectors #45937

Closed
dpinol opened this issue Jul 5, 2022 · 6 comments · Fixed by JuliaSparse/SparseArrays.jl#176
Closed
Labels
sparse Sparse arrays

Comments

@dpinol
Copy link

dpinol commented Jul 5, 2022

In julia 1.6, typeof(length(SparseArray))always returned an Int64.
But in julia 1.7, after #39645 it returned the same type as the index type.
Now in julia 1.8rc1, the type is again always an Int64.
The regression was introduced by #41510, since length(array) is calculated through prod.

On the other hand, I understand that the promotion may be necessary for n-dimensional arrays, since the length may overflow the type of the individual dimensions. So, if we want to stick to Int64, maybe we could document it in the julia 1.8 release notes?

thanks

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Jul 5, 2022

v1.7.3 was released May 6, 2022, and back then I thought 1.8 imminent, and it would be the last 1.7 release.

I realize 1.8 isn't released, so if understand correctly you want 1.7 to be fixed. If 1.8 is released (soon), does it then matter? [I see activity hours ago on "Backports for 1.8-rc2/1.8.0" on the backport branch. So rc2 might be imminent, or just 1.8.0? But I also see "backport 1.7" label being applied 4 days ago, so might not be too late (is that just done out of habit, with the others?).]

Julia 1.7 will then no longer be the stable supported version (unless if made LTS...).

@dpinol
Copy link
Author

dpinol commented Jul 5, 2022

If we commit to returning Int64, I'm happy to have it written somewhere, in the documentation or the release notes.
At least to avoid that developers get confused since according to #39645 it looks like the correct behaviour is returning the type of the index. thanks

@dkarrasch
Copy link
Member

dkarrasch commented Jul 7, 2022

#39645 addresses SparseVectors only, right, so I think we should just specialize

Base.length(x::SparseVector) = x.n

(EDIT: just like we have a specialization size(x::SparseVector) = (getfield(x, :n),).)

Then the return type of length will be fully inferrable. Such a change is fairly specific and shouldn't have "undesired" side-effects, so should be backportable.

@dkarrasch
Copy link
Member

Wait, can you provide an example of what you think changed or regressed? On v1.8rc1 I get

julia> using SparseArrays, Test

julia> y = SparseVector(Int128(8), Int128[4], [5])
8-element SparseVector{Int64, Int128} with 1 stored entry:
  [4]  =  5

julia> @test y isa SparseVector{Int,Int128}
Test Passed

julia> @test @inferred size(y) == (@inferred(length(y))::Int128,)
Test Passed

julia> typeof(length(y))
Int128

Note that #39645 did not touch any SparseMatrixCSC stuff.

@dkarrasch dkarrasch added the sparse Sparse arrays label Jul 7, 2022
@dpinol
Copy link
Author

dpinol commented Jul 7, 2022

@dkarrasch The regression is only when the size of the index is smaller than 64 bit (probably due to prod's type promotion)

Julia 1.8

julia> y = SparseVector(8, Int32[4], [5])
8-element SparseVector{Int64, Int32} with 1 stored entry:
  [4]  =  5

julia> typeof(length(y))
Int64

Julia 1.7

julia> y = SparseVector(8, Int32[4], [5])
8-element SparseVector{Int64, Int32} with 1 stored entry:
  [4]  =  5

julia> typeof(length(y))
Int32

@dkarrasch
Copy link
Member

Thanks, then my proposal above should work.

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

Successfully merging a pull request may close this issue.

3 participants