Skip to content

Conversation

@jumerckx
Copy link
Collaborator

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.

@jumerckx jumerckx requested a review from avik-pal December 12, 2025 17:21
@jumerckx jumerckx self-assigned this Dec 12, 2025
@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")
Copy link
Collaborator

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

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?

Copy link
Collaborator Author

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?

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.

4 participants