Stable-Baselines-Team / stable-baselines3-contrib

Contrib package for Stable-Baselines3 - Experimental reinforcement learning (RL) code
https://sb3-contrib.readthedocs.io
MIT License
442 stars 166 forks source link

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

Open EloyAnguiano opened 3 months ago

EloyAnguiano commented 3 months ago

❓ 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

araffin commented 3 months ago

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

EloyAnguiano commented 3 months ago

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)

araffin commented 3 months ago

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)

EloyAnguiano commented 3 months ago

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