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.

Sign up for GitHub

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

Jump to bottom

Inferring Box scalar shape #2911

Merged
merged 18 commits into from
Jun 29, 2022
Merged

Inferring Box scalar shape #2911

merged 18 commits into from
Jun 29, 2022

Conversation

pseudo-rnd-thoughts
Copy link
Contributor

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented Jun 19, 2022 •
edited
Loading

This PR adds several changes to box

  • Changes check that low and high are instances of np.ndarray instead of np.isscalar as this function has a number of weird cases that we don't want, e.g. strings
  • Adds a check on custom shape that each of the elements are np.issubdtype(type(dim), np.integer)) that should allow a tuple of np.int64. However added conversion after that the shape is a tuple of integers. This is a partial change, however, reflects the expected type of shape and don't expect this to affect someone.
  • Adds new shape inference from scalar low and high through checking if the type of low and high are sub dtypes of np.integer or np.floating.
  • Updated check on if to expand the low and high for scalars to remove isscalar due to the problems noted above.
  • Added new tests for box values with valid and invalid types
  • Added new tests for box errors with the expected inputs

@RedTachyon
Copy link
Contributor

Overall I like the idea, but are you sure we want the default shape to be ()? My first intuition is that (1,) would be better, though I'm not sure.

The difference is somewhat subtle, but the main issue I'm finding now is that with zero-dimensional arrays, you can't concatenate them. You can stack them, the output being different (though just by a single empty dimension)

Zero-dim arrays have this weird status kinda between being just a number and being a proper array (even though the type is np.ndarray), so imo it might be better to keep the defaults as a proper "full" array. And of course if someone explicitly wants to use a zero-dim array, they can do it manually.

@RedTachyon
Copy link
Contributor

(also, whatever the decision will be - tests)

@pseudo-rnd-thoughts
Copy link
Contributor Author

I think you are right on the shape being (1,) instead of (). I will change it and add tests

RedTachyon
RedTachyon reviewed Jun 23, 2022
View reviewed changes
gym/spaces/box.py Outdated Show resolved Hide resolved
RedTachyon
RedTachyon reviewed Jun 24, 2022
View reviewed changes
gym/spaces/box.py Outdated Show resolved Hide resolved
tests/spaces/test_box.py Outdated Show resolved Hide resolved
gym/spaces/box.py Outdated Show resolved Hide resolved
gym/spaces/box.py Outdated Show resolved Hide resolved
@pseudo-rnd-thoughts pseudo-rnd-thoughts changed the title Added Box scalar shape Inferring Box scalar shape Jun 28, 2022
@RedTachyon
Copy link
Contributor

Looks good now

@jkterry1 jkterry1 merged commit 7f6effb into openai:master Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedTachyon RedTachyon RedTachyon left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pseudo-rnd-thoughts @RedTachyon @jkterry1