search

JuliaPOMDP / POMDPs.jl

MDPs and POMDPs in Julia - An interface for defining, solving, and simulating fully and partially observable Markov decision processes on discrete and continuous spaces.
http://juliapomdp.github.io/POMDPs.jl/latest/
Other
657 stars 100 forks source link

`FullyObservablePOMDP` broken `obsindex` #480

Closed WhiffleFish closed 1 year ago

WhiffleFish commented 1 year ago

obindex definition wrongly assumes that parametric type order for FullyObservablePOMDP is {S,A,...} when it really is {M,S,A}. Consequently, a method error is always raised.

https://github.com/JuliaPOMDP/POMDPs.jl/blob/cc15fe957d97ec1c41a2415843eebe8157670ab1/lib/POMDPTools/src/ModelTools/fully_observable_pomdp.jl#L6

https://github.com/JuliaPOMDP/POMDPs.jl/blob/cc15fe957d97ec1c41a2415843eebe8157670ab1/lib/POMDPTools/src/ModelTools/fully_observable_pomdp.jl#L17

lassepe commented 1 year ago

Isn't the fix just to define

POMDPs.obsindex(pomdp::FullyObservablePOMDP{<:Any, S, A}, o::S) where {S, A} = stateindex(pomdp.mdp, o)
lassepe commented 1 year ago

In fact, why are there any type parameters at all? Seems like we can just drop them in obsindex

WhiffleFish commented 1 year ago

Yeeeep it does seem unnecessary. The only reason I see to keep the state type parameter is to assert that the observation really is a valid state, preventing it from either failing silently or relying on stateindex to catch the type error.

zsunberg commented 1 year ago

Yeah, we should just get rid of S and A. We might want to accept observations that are not of type S, for example when S is a StaticVector but the user inputs a Vector.

This is an ancient leftover from the early days of Julia where you either needed those parameters or the coders (i.e. me) did not realize you didn't need them.

WhiffleFish commented 1 year ago

fixed by #485