search

Farama-Foundation / SuperSuit

A collection of wrappers for Gymnasium and PettingZoo environments (being merged into gymnasium.wrappers and pettingzoo.wrappers
Other
452 stars 57 forks source link

Fixed black death wrapper #134

Closed benblack769 closed 2 years ago

benblack769 commented 2 years ago

Resolves #133

RedTachyon commented 2 years ago

So apparently here be dragons.

As I understand, this is a total remake of the black death wrapper, yes? (basically meaning that whatever diff is irrelevant, and it should be analyzed in isolation from the previous version)

I can't actually test it right now, but from a cursory glance over the code, here are some potential pitfalls:

  1. L8 checks whether the env has env.possible_agents, but then the wrapper doesn't use that. Maybe it should be used in place of self.env.agents in L23? This would support agents who die and who have yet to be spawned, if at all.
  2. Info will have an empty dictionary for each "dead" agent - is this the standard PZ convention? (I'm not sure)
  3. Episode ends when all agents end - I assume this is intended

Are there any specific reasons why you're suspicious of this code?

benblack769 commented 2 years ago

@RedTachyon This is the problem: #133 .

benblack769 commented 2 years ago

Overall, this rewrite should be very similar to the old environment, but without the bug. But I don't want to waste time pawing through that old code.

I've spent way too much time looking at that old code, and I don't want people to spend time reasoning about crazy edge cases there, especially if it doesn't work, especially if all the use cases for this wrapper are parallelizable environments anyways, hence the rewrite.

  1. Good point about possible_agents. I think I changed my mind half way through this implementation. Changed to check the observation spaces on reset.
  2. There is no convention for dead agents. If there is any, this is the place to define the convention. We are open to suggestions, if you have any. Right now, the only way to check if the agent is dead is to check if the observation is all 0, which may be a bit dubious for some environments.
  3. Yes, this is the idea.
RedTachyon commented 2 years ago

133 is with the previous version of the wrapper though, right? Or is the same issue present here?

I agree that the previous implementation was... tricky. But this seems to be pretty straight forward since we can skip all of the AEC stuff

benblack769 commented 2 years ago

Yeah, that was with the previous implementation. There is no particular problem I know about the current implementation, other than the obvious history of problems.