RoboticExplorationLab / TrajectoryOptimization.jl

A fast trajectory optimization library written in Julia
https://roboticexplorationlab.org
MIT License
322 stars 62 forks source link

Copy undefined for RobotDynamics models #71

Closed jbwillis closed 2 years ago

jbwillis commented 2 years ago

I’m getting an error running code that was previously working.

ERROR: LoadError: MethodError: no method matching copy(::SingleIntegrator{4})

This happens when I call Problem() with a single model

si_prob = Problem(si_model, si_obj, x0, tf, xf=xT, constraints=si_cons)

From looking at the git blame, it looks like the Problem interface changed so that a single model gets copied into a vector of models. https://github.com/RoboticExplorationLab/TrajectoryOptimization.jl/blob/0e93ad39b5b316f2f7792b1ca16ead348c5acbf9/src/problem.jl#L115

My original issue was with a custom model. A quick test on a model from RobotZoo shows a similar problem.

julia> using RobotZoo

julia> m = RobotZoo.Quadrotor();

julia> copy(m)
ERROR: MethodError: no method matching copy(::RobotZoo.Quadrotor{Rotations.UnitQuaternion{Float64}, 9})
Closest candidates are:
  copy(::SubArray{var"#s814", var"#s813", var"#s812", I, L} where {var"#s814", var"#s813", var"#s812"<:Union{SparseArrays.AbstractSparseMatrixCSC, SparseArrays.SparseVector}, I, L}) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/SparseArrays/src/sparsevector.jl:720
  copy(::SubArray) at subarray.jl:70
  copy(::LinearAlgebra.Cholesky) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/cholesky.jl:439
  ...
Stacktrace:
 [1] top-level scope
   @ REPL[7]:1
jbwillis commented 2 years ago

Looks like the issue is copy is not defined for DiscreteDynamics in RobotDynamics.jl. It is defined for DiscretizedDynamics:

https://github.com/RoboticExplorationLab/RobotDynamics.jl/blob/2b1ae08d0574b183b45fc6accd49091ff9889ace/src/discretized_dynamics.jl#L194

function Base.copy(model::DiscretizedDynamics)
    DiscretizedDynamics(model.continuous_dynamics, model.integrator)
end

function Base.deepcopy(model::DiscreteDynamics)
    DiscreteDynamics(copy(model.continuous_dynamics), copy(model.integrator))
end

I think there is another bug here with the definition of deepcopy. I believe it should be for model::DiscretizedDynamics since DiscreteDynamics models don't have continuous_dynamics members.

bjack205 commented 2 years ago

This behavior is actually intentional, and follows the general behavior of Julia, which does not implement a default copy for user-defined types. The fix here is to define Base.copy for your type.

One option to make the interface a little nicer would be to issue a warning and then simply duplicate the model at every time step.

And you're correct that's a bug in the deepcopy method for DiscreteDynamics. That should be DiscretizedDynamics. It's been fixed now in the main branch of RobotDynamics.

jbwillis commented 2 years ago

Ok, that makes sense. With that said, should the models in RobotZoo have copy defined for them?

bjack205 commented 2 years ago

yeah, it's probably a good idea. I'll make an issue. But in practice you don't typically need it, because Problem won't call copy on a ContinuousDynamics model, only on a DiscreteDynamics model.

I remember why I wanted to use copy in the Problem constructor: in the case the discretized dynamics with an integrator, in the event we eventually parallelize things you need separate copies of the caches in the integrators to avoid race conditions.

I think the best option right now would be to try adding a more helpful error message in the Problem constructor to tell the user to implement Base.copy for their custom DiscreteDynamics model.