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

@set with slices or broadcasting #75

Closed
Jollywatt opened this issue Sep 4, 2022 · 5 comments
Closed

@set with slices or broadcasting #75

Jollywatt opened this issue Sep 4, 2022 · 5 comments

Comments

@Jollywatt
Copy link

Hello,
If I understand correctly, we may expect this to work out-of-the-box:

julia> using Accessors, StaticArrays

julia> s = SVector{4}(1:4) # immutable fixed size vector
4-element SVector{4, Int64} with indices SOneTo(4):
 1
 2
 3
 4

julia> @set s[2] += 100 # single indexing works
4-element SVector{4, Int64} with indices SOneTo(4):
   1
 102
   3
   4

julia> @set s[2:3] .+= 100 # broadcasted functions are not recognised
ERROR: UndefVarError: .+ not defined
Stacktrace:
 [1] top-level scope
   @ REPL[257]:1

julia> @set s[2:3] += [100, 100] # slicing fails ungracefully
ERROR: StackOverflowError:

Regarding the first error, the macro expands to something containing Accessors.:.+. We probably need to esc the operator symbol .+ so it gets evaluated to Base.Broadcast.BroadcastFunction(+), but I’m not sure if it’s that simple.

The second error seems like it needs more urgent attention. If s = [1, 2, 3] is a normal vector, @set s[2:3] += [100, 100] works fine, so it seems like this should be supported for static vectors.

@Jollywatt Jollywatt changed the title Full support for array indexing @set with slices or broadcasting Sep 4, 2022
@jw3126
Copy link
Member

jw3126 commented Sep 4, 2022

Thanks a lot for reporting! Broadcasting is tricky and not supported right now. Though I agree it would be nice to have. The

julia> @set s[2:3] += [100, 100] # slicing fails ungracefully
ERROR: StackOverflowError:

is a bug I would say. The ideal approach to fixing it would be to fix the following in StaticArrays.jl:

julia> Base.setindex(@SVector[1,2,3], [1,2], 1:2)
ERROR: StackOverflowError:
Stacktrace:
 [1] setindex(a::SVector{3, Int64}, x::Vector{Int64}, inds::UnitRange{Int64}) (repeats 79984 times)
   @ StaticArrays ~/.julia/packages/StaticArrays/6QFsp/src/deque.jl:198

and wait for this PR to land.
If you want to get this fixed faster I am happy to guide you.

@aplavin
Copy link
Member

aplavin commented Sep 4, 2022

and wait for this JuliaLang/julia#46453 to land.

I don't think this is necessary. Just fixing StaticArrays setindex should be enough - it's just called in Accessors as is:

@inline setindex(a::StaticArrays.StaticArray, args...) = Base.setindex(a, args...)

@jw3126
Copy link
Member

jw3126 commented Sep 4, 2022

Ah thanks you are right @aplavin

@aplavin
Copy link
Member

aplavin commented Feb 28, 2024

Broadcasting is now supported (Accessors#master, to be released soon)! All examples from the first post work on Vectors.
The remaining SVector issues are because of their setindex method that leads to StackOverflow - yes, it is still the case. When that is fixed, Accessors will support it without any further changes.
Would be useful if you opened an issue in the StaticArrays repo!

@aplavin aplavin closed this as completed Feb 28, 2024
@Jollywatt
Copy link
Author

You might have to be the one to open that StaticArrays issue, @aplavin — I've completely forgotten what this is all about!

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

No branches or pull requests

3 participants