Make length type match index type in sparse vectors

[dkarrasch]

This can be considered a bugfix. This change should be harmless. The type of the n field is as concrete now (Ti) as it used to be (Int). Marking for backport tentatively.

Closes #39644.

[kimikage]

Doesn't this also affect the cases with Ti smaller than Int?
Also, for performance (e.g. inference) reasons, it may be implicitly assumed that size etc. use Int.

[dkarrasch]

Also, for performance (e.g. inference) reasons, it may be implicitly assumed that size etc. use Int.

I'm not sure there is something like implicit type assumptions. size is inferred, so you work with what you get from inference. In any case, I don't believe calls to size (as long as they are inferrable) constitute some kind of bottleneck.

Doesn't this also affect the cases with Ti smaller than Int?

Yes. Do you see any problem with that? I mean, something we should consider or test?

[kimikage]

Yes. Do you see any problem with that?

I'm not sure it's a problem in the wild use cases, but the error below is essentially the same as the issue #39644. It doesn't seem to be an edge case, at least compared to the case where Int128 is used.

julia> SparseVector(1000, [0x4], [5.0])
ERROR: InexactError: trunc(UInt8, 1000)

[dkarrasch]

I see. But this is the same error we have when the length exceeds the length type. For instance, we also have currently:

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

julia> kron(y, y)
ERROR: InexactError: trunc(Int32, 6442450945)

I guess the question is whether we can have common operations that would blow up n and start to fail once it's of type Int32. So, the following works:

y = SparseVector(typemax(Int32), [4], [5])
A = spzeros(Int64(typemax(Int32))+1,typemax(Int32))
A*y # 2147483648-element SparseVector{Float64, Int64} with 0 stored entries

The point is that there is some limit no matter if we set n::Int or n::Ti. But I agree this might be tricky business.

[ViralBShah]

Such tricky business!

[dkarrasch]

There's #37741, which seems to suggest it should be fine to return a non-Int length? @dlfivefifty @timholy

[dlfivefifty]

Yes, that PR makes it official that length does not need to be an Int, which already existed:

julia> typeof(length(1:BigInt(100_000_000)^1000))
BigInt

[kimikage]

The "return" type doesn't matter. It can be converted to promote_type(Int, Ti) in the overloaded method if necessary. (I think that is necessary for compatibility.)

[kimikage]

IMO, the following statement (from #39644 (comment)) is "false".

the index range is the same as the length of the sparse vector.

In other words, the type mismatch itself is not a "bug" that should be fixed. If we make a change, it is a specification change.

The inability to create a sparse array larger than typemax(Int) is one problem. However, the huge array and the type mismatch are orthogonal problems. We haven't had any "practical" discussion of the huge array problem yet.

[dkarrasch]

I have no preference regarding this change, but I'm also not sure I follow. If you have indices of some type Ti, then you can technically have index values up to typemax(Ti), which implies that the length of the vector is at least as much. Now a user invests in a larger index type, and still fails because we tell him/her that we can't store the length, while I'm positive that we can do all sorts of linear algebra with it? Sounds to me like a silly point to fail. I agree that "bugfix" was overly enthusiastic, though.

[kimikage]

If the problem is that the user has false expectations, then we can warn against Ti less than Int. (I don't think that's a good idea from a compatibility point of view, though.)

Integer literals are interpreted as Int by default, and Int is large enough for many use cases. Moreover, dense arrays with typemax(Int) size are practically impossible to handle. Do we also ban the use of Int as Ti due to the false expectations? 😄

In any case, the relationship between the problem and the solution should be clear. For another problem, there may be another, better solution.

[KristofferC]

Removed backport label since this feels a bit iffy to backport this late into the release process.

[dkarrasch]

Shall we close it together with #39644, or are there good reasons to do it? I agree that the exact implications are hard to overlook, so this might be a bit dangerous. Do we consider a PkgEval run worth it?

[ViralBShah]

Just to be on the safe side, we should do a PkgEval run and merge it if it looks good. We probably shouldn't backport it though.

[ViralBShah]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo /run/media/system/data/nanosoldier/cset/bin/cset shield -e su nanosoldier -- -c /run/media/system/data/nanosoldier/workdir/jl_jwYHlH/benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @christopher-dG

[ViralBShah]

Oops ran the wrong one (but I suppose the benchmarks are broken currently?)

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[ViralBShah]

Can't quite tell if the errors are related to this PR - but it may be worthwhile to rebase on master and then run pkgeval again.

[dkarrasch]

@nanosoldier runtests(ALL, vs = ":master")

[dkarrasch]

For all I can say, I coulnd't find any failure (in the first evalpkg run) that seemed related.

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[dkarrasch]

Again, none of the failures look related.