-
Notifications
You must be signed in to change notification settings - Fork 39
StructArrays extension #1966
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
base: main
Are you sure you want to change the base?
StructArrays extension #1966
Conversation
| @safetestset "OneHotArrays" include("integration/onehotarrays.jl") | ||
| @safetestset "AbstractFFTs" include("integration/fft.jl") | ||
| @safetestset "SpecialFunctions" include("integration/special_functions.jl") | ||
| @safetestset "StructArrays" include("integration/structarrays.jl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a test dependency on structarrays
| idx = idx_ref[] + 1 | ||
|
|
||
| scalar_args = [@allowscalar(arg[idx]) for arg in args] | ||
| scalar_args = [@allowscalar((@inbounds arg[idx])) for arg in args] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a specific reason we need inbounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without, I run into:
ERROR: ArgumentError: unable to check bounds for indices of type Reactant.TracedRNumber{Int64}
Stacktrace:
[1] checkindex(::Type{Bool}, inds::Base.OneTo{Int64}, i::Reactant.TracedRNumber{Int64})
@ Base ./abstractarray.jl:751
[2] checkbounds
@ ./abstractarray.jl:689 [inlined]
[3] checkbounds
@ ./abstractarray.jl:699 [inlined]
[4] _getindex
@ ~/Reactant2/ext/ReactantStructArraysExt.jl:62 [inlined]
[5] getindex
@ ~/comrade-reactant/StructArrays.jl/src/structarray.jl:345 [inlined]
[6] #26
@ ~/.julia/packages/GPUArraysCore/aNaXo/src/GPUArraysCore.jl:206 [inlined]
[7] (::Reactant.TracedUtils.var"#26#28"{…})(arg::StructVector{…})
@ Reactant.TracedUtils ./none:0
[8] iterate
@ ./generator.jl:48 [inlined]
[9] collect
@ ./array.jl:791 [inlined]
[10] (::Nothing)(none::typeof(collect), none::Base.Generator{Vector{…}, Reactant.TracedUtils.var"#26#28"{…}})
@ Reactant ./<missing>:0
[11] call_with_reactant(::typeof(collect), ::Base.Generator{Vector{…}, Reactant.TracedUtils.var"#26#28"{…}})
@ Reactant ~/Reactant2/src/utils.jl:523
[12] __elem_apply_loop_body
@ ~/Reactant2/src/TracedUtils.jl:1101 [inlined]
[13] (::Nothing)(none::typeof(Reactant.TracedUtils.__elem_apply_loop_body), none::Base.RefValue{…}, none::Base.RefValue{…}, none::Base.RefValue{…}, none::Base.RefValue{…}, none::Base.RefValue{…})
@ Reactant ./<missing>:0
[14] getproperty
@ ./Base.jl:49 [inlined]
[15] getindex
@ ./refvalue.jl:59 [inlined]
[16] __elem_apply_loop_body
@ ~/Reactant2/src/TracedUtils.jl:1095 [inlined]
But that probably just means I need to add
checkindex(::Type{Bool, inds, i::TracedRNumber{<:Integer})
Though now I'm confused as to why we're not hitting this in other places?
a290b7b to
c23680c
Compare
For now I've only looked at StructArrays of NamedTuples, as this is what Comrade uses, and what would be useful if we start using StructArrays in Reactant itself.