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

Fix type hints errors in gymnasium/spaces #327

Merged
merged 10 commits into from
Feb 13, 2023

Conversation

vcharraut
Copy link
Contributor

@vcharraut vcharraut commented Feb 12, 2023

Description

Removed reportInvalidTypeVarUse and reportGeneralTypeIssues with pyright and fixed the new raised issues.

Files that are covered without errors:

  • init.py
  • box.py
  • dict.py
  • discrete.py
  • graph.py
  • multi_binary.py
  • multi_discrete.py
  • sequence.py
  • space.py
  • text.py
  • tuple.py
  • utils.py

Type of change

Please delete options that are not relevant.

  • 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 commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • 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

@vcharraut vcharraut changed the title fix type hitting errors Fix type hints errors in gymnasium/spaces Feb 12, 2023
@vcharraut vcharraut marked this pull request as ready for review February 12, 2023 23:40
gymnasium/spaces/dict.py Outdated Show resolved Hide resolved
gymnasium/spaces/dict.py Outdated Show resolved Hide resolved
gymnasium/spaces/dict.py Show resolved Hide resolved
gymnasium/spaces/discrete.py Outdated Show resolved Hide resolved
gymnasium/spaces/multi_binary.py Outdated Show resolved Hide resolved
gymnasium/spaces/text.py Outdated Show resolved Hide resolved
gymnasium/spaces/tuple.py Outdated Show resolved Hide resolved
gymnasium/spaces/utils.py Outdated Show resolved Hide resolved
gymnasium/spaces/utils.py Outdated Show resolved Hide resolved
return unflatten_x.reshape(-1, *unflatten_space.shape)
elif isinstance(unflatten_space, Discrete):
return np.asarray(np.nonzero(unflatten_x))[-1, :]

Choose a reason for hiding this comment

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

Add an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you be more specific ?

Choose a reason for hiding this comment

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

else:
   raise TypeError("Expected the ... ")

@@ -55,7 +56,7 @@ def is_np_flattenable(self):
"""Checks whether this space can be flattened to a :class:`spaces.Box`."""
return True

def sample(self, mask: MaskNDArray | None = None) -> int:
def sample(self, mask: MaskNDArray | None = None) -> np.int64 | NDArray[np.int64]:

Choose a reason for hiding this comment

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

I still think this should just be np.int64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -58,7 +58,7 @@ def is_np_flattenable(self):
"""Checks whether this space can be flattened to a :class:`spaces.Box`."""
return True

def sample(self, mask: MaskNDArray | None = None) -> npt.NDArray[np.int8]:
def sample(self, mask: MaskNDArray | None = None) -> NDArray[np.bool_ | np.int8]:

Choose a reason for hiding this comment

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

What case causes np.bool_ as sample return type as it should be the same type as class MultiBinary(Space[NDArray[np.int8]]):

Copy link
Contributor Author

@vcharraut vcharraut Feb 13, 2023

Choose a reason for hiding this comment

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

Well, this comes from np_random.integers that has a wild possiblity of type return, like int, bool and ndarray.
I don't know how to deal with it.
Since np_random.integers accepts several type in args, I guess there would be a way to force the return type of the function based on the type of the args.

I have actually the exact same proble in the graph.py and utils.py files: there are some functions that can accept multiples spaces types but when used with specific spaces, the type raised errors (like flatten_space in utils.py.

That is already the same kind of problem I encountered with this part of code #327 (comment) and I ended up splitting the function into 2 functions to handle each type cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, even if self.n has a static type np.int64, this line is raises an error

tmp: np.int64 = self.np_random.integers(self.n)

Expression of type "ndarray[Any, dtype[int64]]" cannot be assigned to declared type "int64"
  "ndarray[Any, dtype[int64]]" is incompatible with "int64"

Choose a reason for hiding this comment

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

Can we cast the data in some way to fix this, otherwise, raise an issue in numpy.

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 will try to look for solutions and do another PR

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

I think this looks good except for the question on integers where we expect a np.int64 but numpy doesn't allow this currently. If there is no way of solving this issue then we can merge with the current implementation.

@vcharraut
Copy link
Contributor Author

I think this looks good except for the question on integers where we expect a np.int64 but numpy doesn't allow this currently. If there is no way of solving this issue then we can merge with the current implementation.

I think it is better to merge everything for now, I reverted the things I didn't solve.
A lot of the type errors left in gymnasium/spaces are due to functioncs that accept multiple distinct signatures. So I will try to do another PR about it

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit f6d41e8 into Farama-Foundation:main Feb 13, 2023
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