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

Update the spaces for complete type hinting (and updates numpy to 1.21) #37

Merged

Conversation

pseudo-rnd-thoughts
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented Oct 6, 2022

A lot of the spaces in Gymnasium have partial type hints, are missing type hints or have incorrect type hints

This PR fixes and adds a number of type hints in Spaces using the np.NDArray type hints that allows the dtype of a numpy array to be specified.

The aim of this PR is to enable the GeneralTypeHint in pyright for future PRs

One of the requirements of this PR is np.typing.NDArray which was introduced in 1.21.0 to numpy.

@pseudo-rnd-thoughts
Copy link
Member Author

This waiting for the python 3.6 PR to be merged

@pseudo-rnd-thoughts pseudo-rnd-thoughts changed the title Update the spaces for complete type hinting Update the spaces for complete type hinting (and updates numpy to 1.21) Oct 9, 2022
@pseudo-rnd-thoughts pseudo-rnd-thoughts mentioned this pull request Oct 27, 2022
17 tasks
Copy link
Member

@RedTachyon RedTachyon left a comment

Choose a reason for hiding this comment

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

There's a bunch of comments, a good chunk of them being rather pedantic (but many of the changes themselves are pedantic as well, so if we're doing it, we need to do it right)

Most notably I don't really like the pattern of putting Something[Any] everywhere, it doesn't add any information and is just additiona visual clutter.

Also many of the comments apply to multiple places in the code, I didn't comment each individual instance to avoid 100+ review comments

gymnasium/spaces/box.py Show resolved Hide resolved
gymnasium/spaces/box.py Outdated Show resolved Hide resolved
gymnasium/spaces/box.py Show resolved Hide resolved
gymnasium/spaces/dict.py Outdated Show resolved Hide resolved
gymnasium/spaces/space.py Outdated Show resolved Hide resolved
gymnasium/spaces/utils.py Show resolved Hide resolved
gymnasium/spaces/utils.py Show resolved Hide resolved
gymnasium/spaces/utils.py Show resolved Hide resolved
gymnasium/spaces/utils.py Show resolved Hide resolved
tests/spaces/test_dict.py Show resolved Hide resolved
Copy link
Member

@RedTachyon RedTachyon left a comment

Choose a reason for hiding this comment

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

One small stylistic comment, I'm fine with everything else (but check the CI, it might have been failing outside of the gitlab/github mess)

gymnasium/spaces/space.py Outdated Show resolved Hide resolved
@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit 37b4c0b into Farama-Foundation:main Nov 15, 2022
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