-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Array-API] Add support for Box space #1399
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
base: array-api
Are you sure you want to change the base?
Conversation
@amacati I've updated the array-api to match main minimising the number of changes between them allow us to see the differences most easily |
Thanks! Looking at the tests and numpy docs, numpy only supports the Array API standard since version 2.0. This should be the reason why build-necessary fails. We'd have to update that dependency, but bumping the numpy requirement is not a small thing. I remember that we got around that by making all Array API features optional when adding the |
I'm starting to believe this is a sort of a Gymnasium 2.0 like feature change.
In the end, I would want to update the whole project to support array-api, though we can of course start with Spaces only first. Thoughts? If we doing this, it would be good to get some more voices involved to check their thoughts as well |
I would definitely be on board with converting everything. The spaces were just a good module to start with. I also agree with the technical dept. Box could be simplified by a lot if This could very well be something for a gymnasium 2.0 release and it would be great to get some more voices from other people to see where the different interests lie at. |
I agree that it'd make more sense as a 2.0 change, and a good opportunity to clean up the tech debt with It's also make typing Box(Space[T]):
def __init__(self, low: T, high: T, seed: SeedType): ...
def contains(x: T) -> bool: ...
def sample() -> T: ... with |
@@ -354,6 +334,8 @@ def test_infinite_space(low, high, shape, dtype): | |||
|
|||
|
|||
def test_legacy_state_pickling(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a legacy fix from like gym v0.21, I think we can remove this
This would be more appropriate for a general discussion on what a 2.0 version should look like, but has there ever been a discussion about making |
If all the implementation is removed from it, I think making |
If we want this to be more of a 2.0 feature I could also draft a version with significantly reduced complexity which would not be backward compatible. But it does not make much sense to iterate more on this PR before we are sure what direction this should take |
Description
This PR is a first shot at making spaces generic over all Array API compatible frameworks (e.g.
numpy
,jax
,torch
, ...).The benefit of this change would be to make gymnasium compatible with the growing list of environments written in
jax
ortorch
. Strictly speaking, theArrayConversion
wrappers (e.g.JaxToTorch
) are already outside of what we can currently express withgymnasium.spaces
: There is no way to let the action and observation spaces reflect that we are expectingjax.Array
s as observations andtorch.Tensor
s as inputs (safe for custom spaces which are rarely used).This PR is also meant as a basis for discussion if things require breaking changes, new core dependencies (i.e.
array_api_compat
would become a core dependency) etc.One fundamental change is that each box is now also linked with a
device
. This allows users to express that e.g. the observation is an array on the GPU. The example below shows how this impactsspace.contains
.Open Challenges
space.sample
: Sampling will require several changes. It does not make sense to seed atorch
space with anumpy
random generatorType of change
Example
With the changes in this PR, it is already possible to run the following:
Checklist:
pre-commit
checks withpre-commit run --all-files
(seeCONTRIBUTING.md
instructions to set it up)