stable-baselines3-contrib
stable-baselines3-contrib copied to clipboard
[Question] Why does MaskablePPO does not mask with some logic with last observation?
❓ Question
At MaskablePPO
class, the change for getting the masks is to ask the environment to provide it by he function get_action_mask
. I can see that the get_action_mask
only gets the environment object as input, but at that point we also have the self._last_obs
variable. To provide the action mask more information about the observation it is facing, It would be interesting to provide that method with the last observation object, isn't it? I am thinking about a game that has some logic that we want to keep and code to prevent the agent making those actions.
I assume that I am not the first thinking this so, is it a performance killer to do like so? Has it something to do with the environment vectorizations?
Checklist
- [X] I have checked that there is no similar issue in the repo
- [X] I have read the documentation
- [X] If code there is, it is minimal and working
- [X] If code there is, it is formatted using the markdown code blocks for both code and stack traces.
It would be interesting to provide that method with the last observation object, isn't it? I am thinking about a game that has some logic that we want to keep and code to prevent the agent making those actions.
You can add that logic in the environment code, no? (that action mask may depend on previous observation or any other variable that represent the current env)
https://github.com/Stable-Baselines-Team/stable-baselines3-contrib/blob/667a789af964578cf65de25d31380566c37d8b4c/sb3_contrib/common/maskable/evaluation.py#L94
Yes, and I am doing that indeed, but the problem with this order of things is that you have to calculate the action mask for time t
usint the observation at t-1
, and changing this order could be useful to code come logic at mask t
with the observation at t
(even at the t=0
case)
this order could be useful to code come logic at mask t with the observation at t (even at the t=0 case)
This is what is currently done, no? (the action mask depends on obs at t)
Yes, you are right. It was a mistake at my environment code.