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

Set `IndexStyle` to `IndexLinear` for `AbstractFill`

Open jishnub opened this issue 3 years ago • 5 comments

Closes #195 by setting the default IndexStyle to IndexLinear for all AbstractFill

Edit: Oddly, this makes certain benchmarks worse, so let's hold off on merging this for a bit

julia> f = Fill(1, (SOneTo(32),));

julia> fo = OffsetArray(f, 4);

julia> @btime sum($fo);
  3.353 ns (0 allocations: 0 bytes) # master
  17.115 ns (0 allocations: 0 bytes) # PR

jishnub avatar Oct 17 '22 10:10 jishnub

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.87%. Comparing base (de36a50) to head (3536c0e). Report is 155 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #196   +/-   ##
=======================================
  Coverage   97.87%   97.87%           
=======================================
  Files           4        4           
  Lines         660      660           
=======================================
  Hits          646      646           
  Misses         14       14           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 17 '22 10:10 codecov[bot]

Btw I take back my justification for IndexCartesian due to OffsetArrays.jl as they always use IndexLinear. Though its a bit inconsistent:

julia> a = OffsetArray([1,2,3],-1)
3-element OffsetArray(::Vector{Int64}, 0:2) with eltype Int64 with indices 0:2:
 1
 2
 3

julia> Base.IndexStyle(typeof(a))
IndexLinear()

julia> a[1]
2

julia> a = OffsetArray(randn(3,4),-1,-2)
3×4 OffsetArray(::Matrix{Float64}, 0:2, -1:2) with eltype Float64 with indices 0:2×-1:2:
 1.39984   0.983014   0.74824    1.19955
 1.32323   0.434697   0.68893   -1.77555
 1.41984  -0.503202  -0.704223  -1.04858

julia> Base.IndexStyle(typeof(a))
IndexLinear()

julia> a[1]
1.3998366656381436

dlfivefifty avatar Oct 17 '22 10:10 dlfivefifty

Nevermind, I see the key is firstindex:

julia> a = OffsetArray([1,2,3],-1)
3-element OffsetArray(::Vector{Int64}, 0:2) with eltype Int64 with indices 0:2:
 1
 2
 3

julia> firstindex(a)
0

julia> a = OffsetArray(randn(3,4),-1,-2)
3×4 OffsetArray(::Matrix{Float64}, 0:2, -1:2) with eltype Float64 with indices 0:2×-1:2:
  0.817174  -0.3924     0.477989  -1.00355
 -1.22104   -0.765553  -0.233384   0.965962
  0.231041  -0.445986   0.781096  -0.257998

julia> firstindex(a)
1

We should make sure we test this behaviour is correct for offset FillArrays.jl

dlfivefifty avatar Oct 17 '22 10:10 dlfivefifty

I've included tests for offset FillArrays, with the axes being Base.IdentityUnitRanges. Tests comparing linear and Cartesian indexing seem to work fine.

jishnub avatar Oct 17 '22 11:10 jishnub

Can you add firstindex tests?

dlfivefifty avatar Oct 17 '22 15:10 dlfivefifty