Skip to content

Conversation

@zuoxingdong
Copy link
Contributor

@zuoxingdong zuoxingdong commented Oct 12, 2018

Because it seems like this seeding function is rarely used.

Since it seems like this seeding function is rarely used.
@pzhokhov
Copy link
Collaborator

while I am in principle in favor of this change (I'd rather have prng state in the env than a separate global one) looks like there are more strings attached (judging by the test failures). Also putting @gdb in the loop - is there a reason to have a gym-global prng (vs local to environment)?

@gdb
Copy link
Collaborator

gdb commented Oct 18, 2018

I think @joschu chose to do it this particular way; I'd originally had the prng always in the environment. I don't recall the exact reasoning.

@joschu
Copy link
Contributor

joschu commented Oct 19, 2018

Unfortunately, I don't remember the reasoning, so I'll leave it up to you @pzhokhov

@pzhokhov
Copy link
Collaborator

okay then... @zuoxingdong, please fix the test failures, and we can merge.

@zuoxingdong
Copy link
Contributor Author

@pzhokhov Previous failures are fixed. Could you help to check what is the current failure ?

gym/envs/algorithmic/tests/test_algorithmic.py .................         [  4%]
gym/envs/box2d/test_lunar_lander.py ..                                   [  5%]
gym/envs/tests/test_determinism.py FERROR: InvocationError for command '/usr/local/gym/.tox/py3/bin/pytest' (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   py3: commands failed

@pzhokhov
Copy link
Collaborator

I think the problem is lack of mujoco license key in the build... Which means tests on all external PR's are failing at the moment. Will fix shortly.

@pzhokhov
Copy link
Collaborator

should be fixed now, please pull upstream master into your branch to update the PR

@zuoxingdong
Copy link
Contributor Author

@pzhokhov Now it seems okay

@zuoxingdong
Copy link
Contributor Author

@pzhokhov oops, the current error seems from the random seed

@zuoxingdong
Copy link
Contributor Author

What I do now is to create a numpy random state as a class member within Space, do you think it can be a good idea to support an optional flag to seed the space object independently ?

Or provide a seed() API to the space, so that the user can seed the space object independently.

In specific either

    def __init__(self, shape=None, dtype=None, seed=0):
        import numpy as np # takes about 300-400ms to import, so we load lazily
        self.shape = None if shape is None else tuple(shape)
        self.dtype = None if dtype is None else np.dtype(dtype)
        
        self.np_random = np.random.RandomState(seed)

or

class Space(object):
...
    def seed(self, seed=0):
        self.np_random = np.random.RandomState(seed)

@pzhokhov
Copy link
Collaborator

I am not sure I understand a use case for the seed() method in the API, could you explain?

gym/core.py Outdated
@@ -1,3 +1,5 @@
import numpy as np
Copy link
Collaborator

@pzhokhov pzhokhov Oct 26, 2018

Choose a reason for hiding this comment

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

do we need to import numpy here? looks like Space.__init__ intentionally uses lazy load of numpy

@zuoxingdong
Copy link
Contributor Author

@pzhokhov The motivation here is the user can set the environment and its space objects with different seeds.

@zuoxingdong
Copy link
Contributor Author

@pzhokhov Now it seems works fine.

@zuoxingdong
Copy link
Contributor Author

@pzhokhov Shall we merge this ?

@zuoxingdong
Copy link
Contributor Author

@pzhokhov Conflicts resolved

@pzhokhov pzhokhov merged commit 6497c9f into openai:master Jan 30, 2019
@zuoxingdong zuoxingdong deleted the patch-5 branch March 27, 2019 08:05
zlig pushed a commit to zlig/gym that referenced this pull request Apr 24, 2020
* Delete prng.py

Since it seems like this seeding function is rarely used.

* Update __init__.py

* Update kellycoinflip.py

* Update core.py

* Update box.py

* Update discrete.py

* Update multi_binary.py

* Update multi_discrete.py

* Update test_determinism.py

* Update test_determinism.py

* Update test_determinism.py

* Update core.py

* Update box.py

* Update test_determinism.py

* Update core.py

* Update box.py

* Update discrete.py

* Update multi_binary.py

* Update multi_discrete.py

* Update dict_space.py

* Update tuple_space.py

* Update core.py

* Create space.py

* Update __init__.py

* Update __init__.py

* Update box.py

* Update dict_space.py

* Update discrete.py

* Update dict_space.py

* Update multi_binary.py

* Update multi_discrete.py

* Update tuple_space.py

* Update discrete.py

* Update box.py

* Update dict_space.py

* Update multi_binary.py

* Update multi_discrete.py

* Update tuple_space.py

* Update multi_discrete.py

* Update multi_binary.py

* Update dict_space.py

* Update box.py

* Update test_determinism.py

* Update kellycoinflip.py

* Update space.py
parejadan pushed a commit to parejadan/gym that referenced this pull request Sep 28, 2025
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.

4 participants