search

JuliaSIMD / StrideArrays.jl

Library supporting the ArrayInterface.jl strided array interface.
MIT License
54 stars 9 forks source link

remove unused import of `static_length` #68

Open ranocha opened 1 year ago

ranocha commented 1 year ago

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.

chriselrod commented 1 year ago

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

chriselrod commented 1 year ago

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

ranocha commented 1 year ago

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

Have you seen that before?

chriselrod commented 1 year ago

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

ranocha commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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/StrideArraysCore.jl@v0.4.6...v0.4.7

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 commented 1 year ago

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.

codecov[bot] commented 1 year ago

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 ```diff @@ Coverage Diff @@ ## main #68 +/- ## ======================================= Coverage 56.32% 56.32% ======================================= Files 5 5 Lines 245 245 ======================================= Hits 138 138 Misses 107 107 ``` | [Impacted Files](https://app.codecov.io/gh/JuliaSIMD/StrideArrays.jl/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSIMD) | Coverage Δ | | |---|---|---| | [src/StrideArrays.jl](https://app.codecov.io/gh/JuliaSIMD/StrideArrays.jl/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSIMD#diff-c3JjL1N0cmlkZUFycmF5cy5qbA==) | `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.