Skip to content

Base.length dispatch for ArrayPartition (StaticArrays) #220

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

Merged
merged 5 commits into from
Jul 17, 2022

Conversation

dehann
Copy link
Contributor

@dehann dehann commented Jul 15, 2022

This only works for partition elements that themselves have a length-on-type function, e.g. SVector or SMatrix.

I'm trying to use this line of code from NearestNeighbors.jl (issue on the call length(::Type{ArrayPartition...}}):
https://github.com/KristofferC/NearestNeighbors.jl/blob/fb84d15b96a2d7f94f691db4ff6f42e994194c93/src/tree_data.jl#L14

MWE I'm working to fix:

using Manifolds, NearestNeighbors, Distances, StaticArrays
import Manifolds: ArrayPartition

Base.@kwdef struct DistSE2 <: Distances.Metric
  M = SpecialEuclidean(2)
end 
(d::DistSE2)(p,q) = Manifolds.ManifoldsBase.distance(d.M,p,q)

pts = [ArrayPartition(SA[randn(2)...], SMatrix{2,2}([1 0; 0 1.])) for _ in 1:10]
dSE2 = DistSE2()

bt = NearestNeighbors.BallTree(pts, dSE2)

# adding this PR corrects the error
# Base.length(::Type{<:ArrayPartition{F,T}}) where {F,T <: Tuple} = T.parameters .|> length |> sum

julia> bt = NearestNeighbors.BallTree(pts, dSE2)
ERROR: MethodError: no method matching length(::Type{ArrayPartition{Float64, Tuple{SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}}})
Closest candidates are:
  length(::Union{Base.KeySet, Base.ValueIterator}) at /usr/local/share/julia-1.7.3/share/julia/base/abstractdict.jl:58
  length(::Union{DataStructures.OrderedRobinDict, DataStructures.RobinDict}) at ~/.julia/packages/DataStructures/59MD0/src/ordered_robin_dict.jl:86
  length(::Union{DataStructures.SortedDict, DataStructures.SortedMultiDict, DataStructures.SortedSet}) at ~/.julia/packages/DataStructures/59MD0/src/container_loops.jl:322
  ...
Stacktrace:
 [1] NearestNeighbors.TreeData(data::Vector{ArrayPartition{Float64, Tuple{SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}}}, leafsize::Int64)
   @ NearestNeighbors ~/.julia/packages/NearestNeighbors/VZzTb/src/tree_data.jl:14
 [2] BallTree(data::Vector{ArrayPartition{Float64, Tuple{SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}}}, metric::DistSE2; leafsize::Int64, reorder::Bool, storedata::Bool, reorderbuffer::Vector{ArrayPartition{Float64, Tuple{SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}}})
   @ NearestNeighbors ~/.julia/packages/NearestNeighbors/VZzTb/src/ball_tree.jl:39
 [3] BallTree(data::Vector{ArrayPartition{Float64, Tuple{SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}}}, metric::DistSE2)
   @ NearestNeighbors ~/.julia/packages/NearestNeighbors/VZzTb/src/ball_tree.jl:37
 [4] top-level scope
   @ REPL[11]:1

Currently there are further errors that I'm working separately.


xref: Using Manifolds.jl objects represented as ArrayPartition,

This only works for partition elements that themselves have a length-on-type function, e.g. SVector or SMatrix.
@dehann
Copy link
Contributor Author

dehann commented Jul 15, 2022

cc @mateuszbaran , @Affie

@mateuszbaran
Copy link
Contributor

Yes, that seems necessary for NearestNeighbors.jl. Here is a test that can be added for that method:

p3 = ArrayPartition(SA[1.0, 2.0], MMatrix{2,2}([3.0 4.0; 3.0 5.0]))
@test length(typeof(p3)) == 6

@dehann
Copy link
Contributor Author

dehann commented Jul 15, 2022

Thanks, I added the test too!

dehann and others added 2 commits July 15, 2022 13:46
@dehann
Copy link
Contributor Author

dehann commented Jul 15, 2022

I included the suggestions also thanks

@mateuszbaran
Copy link
Contributor

You're welcome 🙂 .

@dehann dehann changed the title Base.length dispatch for ArrayPartition Base.length dispatch for ArrayPartition (StaticArrays) Jul 15, 2022
Co-authored-by: Christopher Rackauckas <[email protected]>
@ChrisRackauckas ChrisRackauckas merged commit 1dd280b into SciML:master Jul 17, 2022
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.

3 participants