Farama-Foundation/PettingZoo

[Proposal] remove/change assert agent in last()

gnoLLex opened this issue · 3 comments

Proposal

In the AECEnv the method last assert the agent. I would propose to check if the agent is not None or remove the assertion entirely, if the agent_selector cant return invalid agents anyway.

Motivation

I'm currently trying to implement some homogeneous multi agent system, where the a specific AgentID does not matter. I just used int (the index of the agent) as id's. Now if last is called and the agent with id 0is selected the assertion fails even though the id is valid.

Pitch

Check if this is a desired behavior. If not change assert agent to assert agent != None in last of AECEnv or remove the check entirely.

Alternatives

No response

Additional context

Maybe I'm confused about this and I'm just starting to use this PettingZoo, but from having a quick glance at agent_selector I would assume it to always return a valid id.
Also shouldn't the agent_selector also use AgentID as a generic?

Checklist

  • I have checked that there is no similar issue in the repo

Ahhh good catch, yeah for integers assert zero fails. We can change it to assert agent is not None. I believe when a new contributor added support for agent ids to be integers there was a test case added, but if not that also should be done. Is your use case nonstandard in any other way that we could test for to be more comprehensive?

Our code coverage turns out to be over 90% so we are testing most parts of the code but there’s definitely some edge cases that aren’t covered

For now I don't think there is a need for any other tests.

Cool let me know if you encounter any other issues then