Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some work on supporting non-standard indexing #44

Merged
merged 8 commits into from
Feb 12, 2021
Merged

Conversation

mateuszbaran
Copy link
Collaborator

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

Copy link
Contributor

@ranocha ranocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/indexing.jl Outdated
@@ -1,5 +1,5 @@
@inline function getindex(sa::HybridArray{S}, ::Colon) where S
return HybridArray{S}(getindex(sa.data, :))
return getindex(sa.data, :)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/indexing.jl Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a good idea.

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
@mateuszbaran
Copy link
Collaborator Author

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
Copy link
Contributor

ranocha commented Feb 9, 2021

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
Copy link
Contributor

ranocha commented Feb 12, 2021

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
Copy link
Collaborator Author

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 🙂 .

@mateuszbaran
Copy link
Collaborator Author

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

@mateuszbaran
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

Copy link
Contributor

@ranocha ranocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 Thanks!

@mateuszbaran
Copy link
Collaborator Author

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

@ranocha
Copy link
Contributor

ranocha commented Feb 12, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants