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

hyperractangles' compute_bbox seems to break KDTree due to setindex() for non StaticArrays types #204

Closed
AbdAlazezAhmed opened this issue Sep 22, 2024 · 1 comment · Fixed by #205
Assignees

Comments

@AbdAlazezAhmed
Copy link

Last release broke something in PointEvalHandler in Ferrite.jl, I noticed that from CI in termi-official/Thunderbolt.jl#153
MWE:

julia> grid = generate_grid(Quadrilateral, (20,20))
Grid{2, Quadrilateral, Float64} with 400 Quadrilateral cells and 441 nodes

julia> KDTree(reinterpret(Vec{2,Float64}, getnodes(grid)))
ERROR: MethodError: no method matching setindex(::Vector{Float64}, ::Float64, ::Int64)

Closest candidates are:
  setindex(::StaticArrays.SHermitianCompact{N, T, L}, ::Any, ::Int64) where {N, T, L}
   @ StaticArrays ~/.julia/packages/StaticArrays/MSJcA/src/SHermitianCompact.jl:125
  setindex(::StaticArraysCore.StaticArray, ::Any, ::Int64)
   @ StaticArrays ~/.julia/packages/StaticArrays/MSJcA/src/deque.jl:185
  setindex(::StaticArraysCore.StaticArray, ::Any, ::Any...)
   @ StaticArrays ~/.julia/packages/StaticArrays/MSJcA/src/deque.jl:198
  ...

Stacktrace:
 [1] build_KDTree(index::Int64, data::Base.ReinterpretArray{…}, data_reordered::Vector{…}, hyper_rec::NearestNeighbors.HyperRectangle{…}, split_vals::Vector{…}, split_dims::Vector{…}, indices::Vector{…}, indices_reordered::Vector{…}, range::UnitRange{…}, tree_data::NearestNeighbors.TreeData, reorder::Bool)
   @ NearestNeighbors ~/.julia/dev/NearestNeighbors/src/kd_tree.jl:134
 [2] KDTree(data::Base.ReinterpretArray{…}, metric::Euclidean; leafsize::Int64, storedata::Bool, reorder::Bool, reorderbuffer::Vector{…})
   @ NearestNeighbors ~/.julia/dev/NearestNeighbors/src/kd_tree.jl:59
 [3] KDTree
   @ ~/.julia/dev/NearestNeighbors/src/kd_tree.jl:19 [inlined]
 [4] KDTree(data::Base.ReinterpretArray{Vec{2, Float64}, 1, Node{2, Float64}, Vector{Node{2, Float64}}, false})
   @ NearestNeighbors ~/.julia/dev/NearestNeighbors/src/kd_tree.jl:19
 [5] top-level scope
   @ REPL[42]:1
Some type information was truncated. Use `show(err)` to see complete types.

It seems like broadcasting in

function compute_bbox(data::AbstractVector{V}) where {V <: AbstractVector}
mins = mapreduce(identity, (a, b) -> min.(a, b), data; init=fill(Inf,V))
maxes = mapreduce(identity, (a, b) -> max.(a, b), data; init=fill(-Inf,V))
return HyperRectangle(mins, maxes)
end

results in Vector not V. IIUC it should be sth like

 function compute_bbox(data::AbstractVector{V}) where {V <: AbstractVector} 
     mins = mapreduce(identity, (a, b) -> V(min.(a, b)), data; init=fill(Inf,V)) 
     maxes = mapreduce(identity, (a, b) -> V(max.(a, b)), data; init=fill(-Inf,V)) 
     return HyperRectangle(mins, maxes) 
 end 

*Not sure about performance tho, it's just a quick simple fix *
However, even after preserving the type V here setindex is used in many places which is only defined for <:StaticArray IIRC.
If this is an actual issue not just me misusing the interface I believe we can either solve it by checking for immutability like:

    new_maxes = @inbounds (ismutabletype(V) ? setindex! : setindex)(hyper_rec.maxes, split_val, split_dim)

or maybe have setindex dispatched for Vector?
also, IIUC neither setindex nor setindex! is defined for Tensor. So if the goal was to retain V maybe we need to dispatch setindex for Vec.

For now the simplest workaround I use locally is

function compute_bbox(data::AbstractVector{V}) where {V <: AbstractVector}
    mins = mapreduce(identity, (a, b) -> min.(a, b), data; init=fill(Inf,V))
    maxes = mapreduce(identity, (a, b) -> max.(a, b), data; init=fill(-Inf,V))
    return HyperRectangle(SVector{length(mins)}(mins), SVector{length(maxes)}(maxes))
end

Thanks!

@fredrikekre
Copy link
Contributor

See also #203

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 a pull request may close this issue.

3 participants