Some work on supporting non-standard indexing #44

mateuszbaran · 2021-02-09T11:39:36Z

@ranocha I've done some work towards supporting non-standard indexing. Could you take a look?

ranocha

Looks mostly good to me 👍

I just noticed that slicing HybridArraywith statically known size produces anSArray`, e.g.

julia> u_array = rand(2, 3, 4); u_hybrid = HybridArray{Tuple{2, 3, HybridArrays.Dynamic()}}(copy(u_array));

julia> u_hybrid[:, :, 1]
2×3 StaticArrays.SArray{Tuple{2,3},Float64,2,6} with indices SOneTo(2)×SOneTo(3):
 0.491955  0.926182  0.880438
 0.591433  0.132179  0.574867

Is this intended behavior? Without further thinking about it, I would have expected to get a HybridArray{Tuple{2,3}} from this operation, which would be mutable. It somehow feels a bit strange to get something immutable from slicing (copying a part of) a mutable array.

ranocha · 2021-02-09T13:26:19Z

src/indexing.jl

@@ -1,5 +1,5 @@
 @inline function getindex(sa::HybridArray{S}, ::Colon) where S
-    return HybridArray{S}(getindex(sa.data, :))
+    return getindex(sa.data, :)


We will lose any static size information by doing this. Is there a way to prevent this? My first impression is that it might be cleaner to always return some HybridArray from this operation, either with Dynamic size (if necessary) or the statically known size (if possible). Maybe something along the lines of hasdynamic(S) and tuple_nodynamic_prod(S) could help?

HybridArray without any statically known sizes doesn't provide any benefits actually, and if all dimensions are statically known then SizedArray from StaticArrays should be used (because it can take advantage of all existing StaticArray methods). In this case, getindex with : can result only in an array without statically known sizes (where HybridArray isn't helpful) or all axes of HybridArrays are statically sized, in which case SizedArray should have been used instead.

Okay. So what about doing what you described, i.e. return sa.data[:] if not all sizes are known statically and a SizedArray otherwise?

I'd say that the place where HybridArray is created with all statically known sizes should be modified in the first place but we could also have a special case for that here.

Yes, right, that's true. I was just triggered by the failing test

Compatibility with EllipsisNotation: Test Failed at /home/runner/work/HybridArrays.jl/HybridArrays.jl/test/nonstandard_indices.jl:19 Expression: typeof(u_hybrid[..]) == typeof(u_hybrid[:]) Evaluated: HybridArray{Tuple{2,StaticArrays.Dynamic()},Float64,2,2,Array{Float64,2}} == Array{Float64,1}

test/nonstandard_indices.jl

ranocha · 2021-02-09T13:38:41Z

src/indexing.jl

@@ -10,39 +10,53 @@ Base.@propagate_inbounds function getindex(sa::HybridArray{S}, inds::Union{Int,
    _getindex(all_dynamic_fixed_val(S, inds...), sa, inds...)
 end

-Base.@propagate_inbounds function _getindex(::Val{:dynamic_fixed_true}, sa::HybridArray, inds::Union{Int, StaticArray{<:Tuple, Int}, Colon}...)
+@inline function Base._getindex(l::IndexLinear, sa::HybridArray{S}, s::Base.Slice) where S


Do you think it would be good to include some basic information about the indexing process in Base here to explain why this is done (and that it relies on undocumented behavior that can change from release to release)?

Yes, that's a good idea.

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>

mateuszbaran · 2021-02-09T15:10:07Z

I just noticed that slicing HybridArraywith statically known size produces anSArray`, e.g.
julia> u_array = rand(2, 3, 4); u_hybrid = HybridArray{Tuple{2, 3, HybridArrays.Dynamic()}}(copy(u_array));

julia> u_hybrid[:, :, 1]
2×3 StaticArrays.SArray{Tuple{2,3},Float64,2,6} with indices SOneTo(2)×SOneTo(3):
 0.491955  0.926182  0.880438
 0.591433  0.132179  0.574867
Is this intended behavior? Without further thinking about it, I would have expected to get a HybridArray{Tuple{2,3}} from this operation, which would be mutable. It somehow feels a bit strange to get something immutable from slicing (copying a part of) a mutable array.

That's a great question. SArray was originally what I needed for best performance in my code. I can introduce there an additional layer of API so that HybridArray itself is less surprising but so far you're the first person complaining about it 🙂 . BTW, StaticArrays.jl is also somewhat inconsistent about it, returning SArray when the input (or part of the input) is mutable.

Anyway, what I believe should happen here is that it returns SizedArray, not HybridArray.

ranocha · 2021-02-09T15:35:33Z

BTW, StaticArrays.jl is also somewhat inconsistent about it, returning SArray when the input (or part of the input) is mutable.

Well, I didn't dive that deep into this behavior so far 😉

Anyway, what I believe should happen here is that it returns SizedArray, not HybridArray.

That sounds good to me (as a new user of HybridArrays) - if it gives the same performance as using an SArray here for your task.

ranocha · 2021-02-12T07:00:33Z

After thinking a bit more about it, I think it's totally fine to return an SArray from slicing static dimensions of a HybridArray. At least in my possible use cases, when I want to get something mutable, i will always be using views. Thus, the current behavior is totally fine 👍

mateuszbaran · 2021-02-12T10:35:31Z

I'd be fine with getindex defaulting to returning HybridArray or SizedArray instead of SArray for that case as long as there is another function that returns SArray. If you don't need it then it doesn't seem particularly important to solve though 🙂 .

…bridArrays.jl into mbaran/indexing2

mateuszbaran · 2021-02-12T12:02:10Z

I've updated the getindex code, does it look OK to you now?

mateuszbaran · 2021-02-12T12:07:11Z

That failure on 1.3 looks like a bug in EllipsisNotation though? On Julia 1.3:

julia> a = randn(2, 3)
2×3 Array{Float64,2}:
 -0.155822  -0.365016   0.390315 
 -1.3536     0.160612  -0.0588314

julia> a[:]
6-element Array{Float64,1}:
 -0.15582181305933568
 -1.353604684762468  
 -0.3650163553825738 
  0.16061201860891566
  0.39031531138352515
 -0.05883139149425665

julia> a[..]
2×3 Array{Float64,2}:
 -0.155822  -0.365016   0.390315 
 -1.3536     0.160612  -0.0588314

On 1.5:

julia> a = randn(2, 3)
2×3 Array{Float64,2}:
 -1.99061   0.333919  0.649871
 -1.0537   -1.43223   1.69521

julia> a[:]
6-element Array{Float64,1}:
 -1.9906141581780128
 -1.0537017264077428
  0.3339193921096322
 -1.4322293088370968
  0.6498713869479286
  1.6952138277847535

julia> a[..]
6-element Array{Float64,1}:
 -1.9906141581780128
 -1.0537017264077428
  0.3339193921096322
 -1.4322293088370968
  0.6498713869479286
  1.6952138277847535

mateuszbaran · 2021-02-12T12:09:39Z

Ah, ok, EllipsisNotation.jl only supports Julia 1.5+ and is probably broken on <1.5.

ranocha

Looks good to me 👍 Thanks!

mateuszbaran · 2021-02-12T12:38:24Z

You're welcome 🙂 . Do you also need setindex! with custom indexing?

ranocha · 2021-02-12T12:42:05Z

We don't use setindex! with something like .. at the moment. While this might come in handy, it's not necessary for us right now

mateuszbaran added 2 commits February 9, 2021 12:25

some work on supporting non-standard indexing

ff4558e

Merge branch 'master' into mbaran/indexing2

c2ead49

ranocha reviewed Feb 9, 2021

View reviewed changes

Update test/nonstandard_indices.jl

c9d0966

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>

mateuszbaran added 2 commits February 12, 2021 13:00

getindex probably ready?

1d7cd9c

Merge branch 'mbaran/indexing2' of https://github.com/mateuszbaran/Hy…

02c046c

…bridArrays.jl into mbaran/indexing2

mateuszbaran added 2 commits February 12, 2021 13:10

check compat with EllipsisNotation only on Julia 1.5+

51b9433

fix version check?

ed27b89

ranocha approved these changes Feb 12, 2021

View reviewed changes

bump version

cd850bd

mateuszbaran merged commit 2c0c3c7 into master Feb 12, 2021

mateuszbaran mentioned this pull request Feb 24, 2021

Indexing SArray with SArray of CartesianIndex creates an MArray JuliaArrays/StaticArrays.jl#635

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some work on supporting non-standard indexing #44

Some work on supporting non-standard indexing #44

mateuszbaran commented Feb 9, 2021

ranocha left a comment

ranocha Feb 9, 2021

mateuszbaran Feb 9, 2021

ranocha Feb 9, 2021

mateuszbaran Feb 12, 2021

ranocha Feb 12, 2021

ranocha Feb 9, 2021

mateuszbaran Feb 9, 2021

mateuszbaran commented Feb 9, 2021

ranocha commented Feb 9, 2021

ranocha commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

ranocha left a comment

mateuszbaran commented Feb 12, 2021

ranocha commented Feb 12, 2021

Some work on supporting non-standard indexing #44

Some work on supporting non-standard indexing #44

Conversation

mateuszbaran commented Feb 9, 2021

ranocha left a comment

Choose a reason for hiding this comment

ranocha Feb 9, 2021

Choose a reason for hiding this comment

mateuszbaran Feb 9, 2021

Choose a reason for hiding this comment

ranocha Feb 9, 2021

Choose a reason for hiding this comment

mateuszbaran Feb 12, 2021

Choose a reason for hiding this comment

ranocha Feb 12, 2021

Choose a reason for hiding this comment

ranocha Feb 9, 2021

Choose a reason for hiding this comment

mateuszbaran Feb 9, 2021

Choose a reason for hiding this comment

mateuszbaran commented Feb 9, 2021

ranocha commented Feb 9, 2021

ranocha commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

mateuszbaran commented Feb 12, 2021

ranocha left a comment

Choose a reason for hiding this comment

mateuszbaran commented Feb 12, 2021

ranocha commented Feb 12, 2021