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

State-dependent action spaces in POMDP are not transfered to underlying MDP #429

Closed
AlexBork opened this issue Sep 14, 2022 · 3 comments · Fixed by #430
Closed

State-dependent action spaces in POMDP are not transfered to underlying MDP #429

AlexBork opened this issue Sep 14, 2022 · 3 comments · Fixed by #430

Comments

@AlexBork
Copy link
Contributor

The call of an underlying MDP for a POMDP does not preserve state-dependent action spaces, i.e. calling actions(UnderlyingMDP(pomdp), s) for a POMDP with state-dependent action space yields a wrong result.

Adding

POMDPs.actions(mdp::UnderlyingMDP{P, S, A}, s::S) where {P,S,A} = actions(mdp.pomdp, s)

to ModelTools/underlying_mdp.jl fixes the problem.
I propose adding the fix to the main repository.

@zsunberg
Copy link
Member

Hi @AlexBork , thanks for reporting. One issue is that actions(m::POMDP, s) is ill-defined because the agent can't typically tell which state it is in; Thus, actions(m::POMDP, s) instead we use actions(m::POMDP, b). So your proposal might lead to errors if the person who defined the POMDP expects a belief instead of a state.

Another option would be for you to define

POMDPs.actions(::UnderlyingMDP{<:YourPOMDP}, s)

What do you think about that?

If you still think that adding the method you proposed to POMDPTools is a better solution, I'd be happy to accept a PR!

@zsunberg
Copy link
Member

Actually I guess that actions(m::POMDP, s) is advertised as part of the interface (

actions(m::Union{MDP,POMDP}, s)
), so we should make your proposed change.

Can you submit a PR (this would be great practice if you haven't done it before!), or should I make the change?

@AlexBork
Copy link
Contributor Author

Hey @zsunberg, thank you very much for the swift answer. I will submit a PR containing the change.

AlexBork added a commit to AlexBork/POMDPs.jl that referenced this issue Sep 15, 2022
Fixes underlying MDP not preserving the state-dependent actions of POMDP
(See: JuliaPOMDP#429)
zsunberg pushed a commit that referenced this issue Sep 20, 2022
)

* Fix for underlying MDP

Fixes underlying MDP not preserving the state-dependent actions of POMDP
(See: #429)

* Added test
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 a pull request may close this issue.

2 participants