-
Notifications
You must be signed in to change notification settings - Fork 213
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
exposed unity random seed to python api surface #823
Conversation
To add to some context, I much prefer this solution, of setting the random seed globally, rather than every random action including a random seed, which then sets the seed inside of the action. So, instead of: controller.step(action="RandomizeMaterials", randomSeed=42)
controller.step(action="RandomizeMaterials", randomSeed=42) you have controller.step(action="SetRandomSeed", seed=42)
controller.step(action="RandomizeMaterials")
controller.step(action="RandomizeMaterials") This is much more consistent with how one expects to use a random seed in other libraries. For instance, in NumPy, one doesn't use: np.random.randn((125, 125), random_seed=42)
np.random.randn((125, 125), random_seed=45)
np.random.randn((125, 125), random_seed=46) You instead set the seed once globally and keep sampling from it. |
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.
LGTM.
Added a test for it, and visually tested in Colab: https://colab.research.google.com/drive/1QjMJrlXKS-b_o1N2MMbcf_0oTGdaaYCi?usp=sharing
Thanks Matt, I like your suggestion! In the interim, one thing to note is the following sequences of actions will set the random seed twice. controller.step(action="SetRandomSeed", seed=42)
controller.step(action="InitialRandomSpawn") # default randomSeed=0, so the previous line didn't really do anything However, this case should not really cause issues for the end user (behavior should still be reproducible even though the seed switches). |
Another thing to consider is adding a random seed param to the |
I tend to prefer against having a seed in reset/initialize. Some reasons:
import random
controller.step(action="SetRandomSeed", seed=42)
controller.step(action="RandomizeMaterials")
# --- some code that may utilize unity's random function, but the scene has not reset ---
random_n_steps = random.choice(range(5, 20))
for _ in range(random_n_steps):
# assume this is a stochastic movement that randomly samples moveMagnitude in Unity
controller.step(action="MoveAhead")
controller.step(action="SetRandomSeed", seed=42)
controller.step(action="RandomizeLighting") If one only knew about
|
oops this case could be an issue with the current codebase controller.step(action="SetRandomSeed", seed=42)
e1 = controller.step(action="InitialRandomSpawn") # default randomSeed=0, so the previous line didn't really do anything
controller.step(action="SetRandomSeed", seed=41)
e2 = controller.step(action="InitialRandomSpawn") A user might expect |
I think this is more of a fault of |
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.
I am fine with the change with one caveat that we need to ensure that we make users aware that the order that actions are executed in will change the result if the actions invoke the RNG. For example,
SetRandomSeed
RandomizeMaterials
RandomizeMaterials
vs
SetRandomSeed
RandomizeMaterials
SomeOtherRandomAction
RandomizeMaterials
The latter will result in a different set of random materials. This behavior is not different from say using Python's random and explicitly setting the seed. There is also the issue of an action on the Unity side indirectly using the RNG through some other API that we call, which will affect downstream calls to the RNG. This only matters when someone is say generating a dataset and then adds an additional call to an action within a sequence of other actions that use the RNG.
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.
A few comments but I'm otherwise happy with this update.
public void SetRandomSeed(int seed) { | ||
UnityEngine.Random.InitState(seed); | ||
actionFinishedEmit(true); | ||
} |
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.
What about the System.Random
seed?
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.
I've added a new systemRandom
variable to BaseFPSAgentController, which is now used in place of always constructing a new System.Random()
object.
Some notes:
- There was no way to set the seed of
System.Random
globally. System.Random
is necessary for generating doubles, instead of just floats (which can be done withUnityEngine.Random
).- It is intentionally not used with a few actions that have
randomSeed
as a parameter. The only public one is InitialRandomSpawn, which is being reworked, such that, among other things, it will no longer have a random seed parameter. - After calling
controller.reset()
, there are no guarantees that the seeds remain in tact.
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.
LGTM.
Main change is on L3057 in BaseFPSAgentController.cs