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

getindex not implemented on ProductGroup and is there a way to view components? #495

Closed
Affie opened this issue Jun 30, 2022 · 7 comments
Closed

Comments

@Affie
Copy link
Contributor

Affie commented Jun 30, 2022

julia> M = ProductManifold(SpecialEuclidean(2), SpecialEuclidean(2));

julia> G = ProductGroup(M);

julia> p = identity_element(G);

julia> p[G,1]
ERROR: MethodError: no method matching getindex(::ProductRepr{Tuple{ProductRepr{Tuple{Vector{Float64}, Matrix{Float64}}}, ProductRepr{Tuple{Vector{Float64}, Matrix{Float64}}}}}, ::ProductGroup{ℝ, Tuple{GroupManifold{ℝ, ProductManifold{ℝ, Tuple{TranslationGroup{Tuple{2}, ℝ}, SpecialOrthogonal{2}}}, Manifolds.SemidirectProductOperation{RotationAction{TranslationGroup{Tuple{2}, ℝ}, SpecialOrthogonal{2}, LeftAction}}}, GroupManifold{ℝ, ProductManifold{ℝ, Tuple{TranslationGroup{Tuple{2}, ℝ}, SpecialOrthogonal{2}}}, Manifolds.SemidirectProductOperation{RotationAction{TranslationGroup{Tuple{2}, ℝ}, SpecialOrthogonal{2}, LeftAction}}}}}, ::Int64)
Closest candidates are:
  getindex(::ProductRepr, ::ProductManifold, ::Union{Colon, Integer, Val, AbstractVector}) at ~/.julia/packages/Manifolds/WsRwu/src/manifolds/ProductManifold.jl:627
  getindex(::ProductRepr, ::VectorBundle, ::Symbol) at ~/.julia/packages/Manifolds/WsRwu/src/manifolds/VectorBundle.jl:511
Stacktrace:
 [1] top-level scope
   @ REPL[6]:1

I added this to carry on and it seems to work.

Base.@propagate_inbounds function Base.getindex(
    p::ProductRepr,
    M::ProductGroup,
    i::Union{Integer,Colon,AbstractVector,Val},
  )
  return get_component(base_manifold(M), p, i)
end

EDIT: put in the correct stack trace

@kellertuer
Copy link
Member

Thanks, we should definetly do something about this. The one thing we could check is, whether its reasonable to make this more general, i.e. to @trait_function both getindex and setindex! Sith M as first parameter. What do you think @mateuszbaran ?

@mateuszbaran
Copy link
Member

I'll fix it, this definitely should work. I wouldn't use @trait_function though, getindex and setindex! have way too many loosely typed methods for that.

@mateuszbaran
Copy link
Member

Regarding viewing components, do you mean viewing parts of an affine or screw matrix for SE(n)? Because I don't really see how viewing would be useful for ProductRepr or ArrayPartition.

@Affie
Copy link
Contributor Author

Affie commented Jul 1, 2022

Thank you very much for the quick fix.

Regarding viewing components, do you mean viewing parts of an affine or screw matrix for SE(n)? Because I don't really see how viewing would be useful for ProductRepr or ArrayPartition.

I'm trying to improve optimization performance and currently I copy values from a container to the cost function based on the structure of the graph.

I experimented with creating a product manifold with all the variables (tired with 100 for now) but I don't want to copy the data to the cost function every iteration. It works something like this:
With a structure:
x1---f1---x2---f1----x3----f2----x4----f2----x5

the cost is build from the structure, so i would evaluate:
cost = f1(x1,x2) + f1(x2,x3) + f2(x3,x4) + f2(x4,x5)

I previously used (before upgrading to Manifolds.jl) a view, f1(view(vars[x1]), view(vars[x2]), etc.

I hope that is clear enough, that's why I asked how to create a view of the components.

There is a lot of room for improvement, but I'm trying to take small steps at a time.

@mateuszbaran
Copy link
Member

In ProductRepr and ArrayPartition representations getindex doesn't need a view because you get the component without any copying. For the affine matrix representation, you should already get a view.

By the way, could you should me a small-ish example of code you are benchmarking? You don't need to remove AMP/IncrementalInference stuff. Achieving good performance with Manifolds.jl is not always intuitive.

One more tip, if you have a product of something like 100 manifolds, you should try factoring it into a product of a few PowerManifolds by grouping components with the same manifold together. ProductManifold is actually designed to perform the best with up to maybe 10 manifolds? I'm not even sure because I've never needed more than 3.

@mateuszbaran
Copy link
Member

Also keep in mind that you can have things like a product manifold of power manifolds of product manifolds.

@Affie
Copy link
Contributor Author

Affie commented Jul 2, 2022

In ProductRepr and ArrayPartition representations getindex doesn't need a view because you get the component without any copying. For the affine matrix representation, you should already get a view.

Yes, of course, thanks. I was stuck in the way of thinkging pre Manifolds.jl code where I just had a vector and I had to do a view for slices to avoid copies. But when I read your answer I realized it.

By the way, could you should me a small-ish example of code you are benchmarking? You don't need to remove AMP/IncrementalInference stuff. Achieving good performance with Manifolds.jl is not always intuitive.

So far I'm working here JuliaRobotics/IncrementalInference.jl#1546 (comment) (this is one of the factors i'm testing with), so we can perhaps continue the discussion there. I'm still experimenting with how exactly the implementation should look. Therefore, I'll send you an example as soon as I've made enough progress there. I first want to refactor into using this tip you gave:

One more tip, if you have a product of something like 100 manifolds, you should try factoring it into a product of a few PowerManifolds by grouping components with the same manifold together. ProductManifold is actually designed to perform the best with up to maybe 10 manifolds? I'm not even sure because I've never needed more than 3.

That is a great idea. Thanks, I will try it out.

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