Default implementation of convert_s for instances of QuickPOMDPs.QuickMDP

[kevinbradner]

Note: not sure if the following is already covered by #2

I have been experimenting with porting a gridworld-style MDP from python to Julia using https://github.com/JuliaAcademy/Decision-Making-Under-Uncertainty/blob/master/notebooks/1-MDPs.jl as a starting point. In order to run Crux.jl's deep Q-learning solver on a MDP, that MDP needs to have an implementation of POMDPs.convert_s.

Based on JuliaPOMDP/POMDPs.jl#452, I provided the following implementation to get things working:

POMDPs.convert_s(T::Type{<:AbstractArray}, s::State, m::QuickPOMDPs.QuickMDP) = convert(T, vcat(s))

I'm not too familiar with Julia yet, but it seems that with a little modification of the type requirements in the function above, this could become a universal implementation of convert_s for any MDP built using the QuickMDP constructor. It's more or less the same as return in Haskell, for example.

Does this seem reasonable? It's certainly not hard to provide the implementation manually on a case-by-case basis, but a default implementation could save a lot of troubleshooting time for Julia beginners like myself or the OP in that issue linked above.

[zsunberg]

Thanks for posting!

The short answer is that something like what you are suggesting has already been done: https://github.com/JuliaPOMDP/POMDPs.jl/blob/d837f1650c80787ce4ca6a5230813698b9eac9a6/src/pomdp.jl#L153-L159

The reason that the implementation already in POMDPs.jl linked above does not work for the JuliaAcademy MDPs tutorial is that State is not an AbstractArray, even though it seems like it should be (the x and y position seems like it should be a 2 element vector).

So, the best fix would be to make the state in the MDP example a vector. Two ways to do this are:

struct State <: StaticArrays.FieldVector{2, Int}
	x::Int
	y::Int
end

or just using the static vector itself as the state type:

abstract type GridWorld <: MDP{StaticArrays.SVector{2,Int}, Action} end

and deleting State. Then the existing convert_s methods should work.

I am not sure how to update the julia academy code, but such an update would be extremely helpful!

The reason that we do not use the = convert(T, vcat(s)) implementation that you suggested is because it does not produce an intuitive result. For instance, we would want convert_s(Vector, State(1,2), m) to return [1, 2], but with the vcat implementation you suggested, it will return a one-element vector: [State(1,2)].

[kevinbradner]

Thanks, adding that instance of StaticArrays.FieldVector worked! That's an easy tweak to make. It also fixed another bug I was seeing, so maybe that was a consequence of my fix being a vector of states rather than a vectorized state.

There still seems to be at least one bug in my code preventing a successful call to solve (something about a dimension mismatch which I won't have a chance to debug today). Assuming that gets cleared up without further changes to the state representation, I'll make a pull request on the original tutorial notebook.

[kevinbradner]

Whoops, forgot to close the issue. Doing that now.