StrideArrays.jl icon indicating copy to clipboard operation
StrideArrays.jl copied to clipboard

remove unused import of `static_length`

Open ranocha opened this issue 1 year ago • 9 comments

I couldn't find any reference to static_length in StrideArrays.jl. Thus, I removed it to prepare for the update to ArrayInterface.jl v7.

ranocha avatar Feb 24 '23 15:02 ranocha

Okay, but do note that every instance of length are likely to become static_length following the upgrade.

chriselrod avatar Feb 24 '23 17:02 chriselrod

Also, it may be worth moving most of this package into PkgExtensions for StrideArraysCore.

chriselrod avatar Feb 25 '23 04:02 chriselrod

Yeah, I agree. I just wanted to start with something simple to see whether CI passes. Looks like there are two issues

  • Aqua.jl detects a type piracy in https://github.com/JuliaSIMD/StrideArrays.jl/blob/main/src/blas.jl#L6
  • A vcat result is expected to return a static size but yields dynamic one: https://github.com/JuliaSIMD/StrideArrays.jl/actions/runs/4263950919/jobs/7421352107#step:6:1614

Have you seen that before?

ranocha avatar Feb 25 '23 09:02 ranocha

That this type priates StrideArraysCore is expected. Why vcat no longer returns static sizes will need investigating.

chriselrod avatar Mar 06 '23 04:03 chriselrod

Why vcat no longer returns static sizes will need investigating.

Bisected to the update of StrideArraysCore.jl v0.4.6 -> v0.4.7 using the following code

julia> using Pkg; Pkg.activate(temp = true); Pkg.add([
           PackageSpec(name = "StrideArrays", version = "0.1.23"),
           PackageSpec(name = "StrideArraysCore", version = "0.4.7"),
       ]); using StrideArrays

julia> let
       A = @StrideArray rand($(5 << 1), $(1 << 3))
       A_u = view(A, StaticInt(1):StaticInt(6), :)
       A_l = view(A, StaticInt(7):StaticInt(10), :)
       StrideArrays.size(A) === StrideArrays.size(vcat(A_u, A_l))
       end
false # false with v0.4.7, true with v0.4.6

The diff is https://github.com/JuliaSIMD/StrideArraysCore.jl/compare/v0.4.6...v0.4.7

Does this already tell you what's going wrong, @chriselrod?

ranocha avatar Mar 12 '23 11:03 ranocha

That this type priates StrideArraysCore is expected.

I disabled the piracy tests. Since this requires Aqua.jl v0.6, I added compat bounds to test/Project.toml and let CompatHelper track them.

ranocha avatar Mar 12 '23 11:03 ranocha

Why vcat no longer returns static sizes will need investigating.

Bisected to the update of StrideArraysCore.jl v0.4.6 -> v0.4.7 using the following code

julia> using Pkg; Pkg.activate(temp = true); Pkg.add([
           PackageSpec(name = "StrideArrays", version = "0.1.23"),
           PackageSpec(name = "StrideArraysCore", version = "0.4.7"),
       ]); using StrideArrays

julia> let
       A = @StrideArray rand($(5 << 1), $(1 << 3))
       A_u = view(A, StaticInt(1):StaticInt(6), :)
       A_l = view(A, StaticInt(7):StaticInt(10), :)
       StrideArrays.size(A) === StrideArrays.size(vcat(A_u, A_l))
       end
false # false with v0.4.7, true with v0.4.6

The diff is JuliaSIMD/[email protected]

It looks like the views are already broken: StrideArraysCore.jl v0.4.6 (calling a method in StrideArraysCore.jl) gives

julia> A = @StrideArray rand($(5 << 1), $(1 << 3)); A_u = view(A, StaticInt(1):StaticInt(6), :)
6×8 StrideArray{Float64, 2, (1, 2), Tuple{StaticInt{6}, StaticInt{8}}, Tuple{Nothing, StrideArraysCore.StrideReset{StaticInt{10}}}, Tuple{StaticInt{1}, StaticInt{1}}, StrideArraysCore.StaticStrideArray{Float64, 2, (1, 2), Tuple{StaticInt{10}, StaticInt{8}}, Tuple{Nothing, Nothing}, Tuple{StaticInt{1}, StaticInt{1}}, 80}} with indices static(1):static(6)×static(1):static(8):
 0.191039  0.574006   0.290582   0.175618  0.178092   0.196963  0.154767  0.541234
 0.392801  0.599062   0.735296   0.482211  0.151804   0.807586  0.16376   0.118286
 0.738249  0.913247   0.0873968  0.452708  0.473348   0.745417  0.758472  0.778555
 0.749282  0.423333   0.597436   0.805952  0.747627   0.392767  0.287661  0.687246
 0.982385  0.913687   0.759203   0.987565  0.47629    0.21957   0.869044  0.905959
 0.308999  0.0626124  0.609769   0.958194  0.0620535  0.890362  0.411321  0.341948

while StrideArraysCore.jl v0.4.7 (calling a method from Base) yields

julia> A = @StrideArray rand($(5 << 1), $(1 << 3)); A_u = view(A, StaticInt(1):StaticInt(6), :)
6×8 view(::StrideArraysCore.StaticStrideArray{Float64, 2, (1, 2), Tuple{StaticInt{10}, StaticInt{8}}, Tuple{Nothing, Nothing}, Tuple{StaticInt{1}, StaticInt{1}}, 80}, static(1):static(6), :) with eltype Float64 with indices static(1):static(6)×static(1):static(8):
 0.119847  0.649684  0.137111    0.649216  0.176489  0.946151    0.0680405  0.103388
 0.81982   0.332002  0.545564    0.729092  0.635139  0.882212    0.809322   0.018732
 0.730171  0.266168  0.219128    0.748357  0.820015  0.00645693  0.144222   0.591687
 0.881642  0.601935  0.871084    0.367133  0.621668  0.454102    0.662695   0.0535707
 0.572697  0.404497  0.533063    0.946217  0.850804  0.710305    0.0896228  0.98938
 0.981602  0.533421  0.00855031  0.873997  0.469507  0.28981     0.294725   0.866071

ranocha avatar Mar 12 '23 11:03 ranocha

It looks like the change in https://github.com/JuliaSIMD/StrideArraysCore.jl/compare/v0.4.6...v0.4.7#diff-99b463e6d0d4aa026397c8d135183d2e509cf0c20acbf41fe835c05b69dbc269R263 causes the different behavior.

ranocha avatar Mar 12 '23 12:03 ranocha

Codecov Report

Patch and project coverage have no change.

Comparison is base (3a5f3d9) 56.32% compared to head (37ce3e7) 56.32%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #68   +/-   ##
=======================================
  Coverage   56.32%   56.32%           
=======================================
  Files           5        5           
  Lines         245      245           
=======================================
  Hits          138      138           
  Misses        107      107           
Impacted Files Coverage Δ
src/StrideArrays.jl 100.00% <ø> (ø)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Mar 14 '23 02:03 codecov[bot]