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

Correct obs type in parallel_rps doc example #1170

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

dm-ackerman
Copy link
Contributor

The value was an int, but all the testing expects observations to be numpy types. This has been changed with updated comments to explain.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

The value was an int, but all the testing expects obsevations to be
numpy types. This has been changed with updates comments to explain.
observations = {
self.agents[i]: int(actions[self.agents[1 - i]])
self.agents[i]: np.array(actions[self.agents[1 - i]], dtype=np.int_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is np.int_ the correct dtype?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it looks like np.int64 may be the most correct choice. int_ is the same on most platforms, but maybe not all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah int64 is what I've seen elsewhere in the code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could fix this I can merge it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a fix the pr

correct type and update name to be less confusing
@dm-ackerman
Copy link
Contributor Author

I updated the type to int64, and made the initial state a bit more clear

@elliottower elliottower merged commit 3f8f1be into Farama-Foundation:master Feb 14, 2024
47 checks passed
@dm-ackerman dm-ackerman deleted the doctest_fix branch May 6, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants