stable-baselines3-contrib icon indicating copy to clipboard operation
stable-baselines3-contrib copied to clipboard

[Question] Why does MaskablePPO does not mask with some logic with last observation?

Open EloyAnguiano opened this issue 3 months ago • 4 comments

❓ 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

EloyAnguiano avatar Mar 21 '24 08:03 EloyAnguiano

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

araffin avatar Mar 31 '24 10:03 araffin

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)

EloyAnguiano avatar Apr 03 '24 11:04 EloyAnguiano

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)

araffin avatar Apr 03 '24 11:04 araffin

Yes, you are right. It was a mistake at my environment code.

EloyAnguiano avatar Apr 05 '24 07:04 EloyAnguiano