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

Change discrete dtype to np.int64 #141

Merged

Conversation

pseudo-rnd-thoughts
Copy link
Member

The Discrete class definition return type is int while the space dtype is np.int64.
This PR makes the Discrete type consistent with np.int64.

@younik
Copy link
Member

younik commented Nov 18, 2022

Side question: do we need np.int64 instead of np.int32?

I don't have an example in mind where you need int64 for Discrete; while usually int32 is used

@pseudo-rnd-thoughts
Copy link
Member Author

Agree, this was my initial thoughts as well but the numpy random integers and choice functions return np.int64 and the dtype was np.int64 already.

@younik
Copy link
Member

younik commented Nov 18, 2022

Agree, this was my initial thoughts as well but the numpy random integers and choice functions return np.int64 and the dtype was np.int64 already.

If we want to do this, we can change it by passing dtype to integers and an explicit arange to choice

@pseudo-rnd-thoughts
Copy link
Member Author

@younik I have decided not to change it for three reasons

  1. When you convert python int to numpy, its type is np.int64
  2. numpy functions default dtype for integers is int64
  3. MultiDiscrete uses np.int64

Therefore, Im going to keep it to np.int64 and it minimise the difference for backward compatibility.

@@ -21,9 +21,9 @@ class Discrete(Space[int]):

def __init__(
self,
n: int,
n: int | np.int64,
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use one of the general numpy dtypes to allow int/int64/int32/whatever other int? The annotation could be int | np.integer, then I think it would accept int, int64, int32, uint8 and all of that stuff. While it does make sense to narrow down the return type, this would throw redundant errors (since all of those can be cast to int64 anyways)

Copy link
Member Author

@pseudo-rnd-thoughts pseudo-rnd-thoughts Nov 28, 2022

Choose a reason for hiding this comment

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

Yes, I can edit to use np.integer[Any] and this should cover all of the cases mentioned

Copy link
Member

Choose a reason for hiding this comment

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

Also in L24

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit 6f139cd into Farama-Foundation:main Nov 29, 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.

3 participants