Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task done
gnoLLex opened this issue Oct 18, 2023 · 3 comments · Fixed by #1120
Closed
1 task done

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

gnoLLex opened this issue Oct 18, 2023 · 3 comments · Fixed by #1120
Labels
enhancement New feature or request

Comments

@gnoLLex
Copy link

gnoLLex commented Oct 18, 2023

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
@gnoLLex gnoLLex added the enhancement New feature or request label Oct 18, 2023
@elliottower
Copy link
Contributor

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

@gnoLLex
Copy link
Author

gnoLLex commented Oct 18, 2023

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

@elliottower
Copy link
Contributor

Cool let me know if you encounter any other issues then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants