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

RL4J - Changed to use Nd4j Random instead of java.util.Random #8282

Merged
merged 2 commits into from Oct 16, 2019

Conversation

@aboulang2002
Copy link
Contributor

commented Oct 9, 2019

What changes were proposed in this pull request?

Several places in RL4J uses java.util.Random and does not use the seed. This makes it impossible to have identical results with identical runs. Plus java.util.Random makes it very difficult to unit test components that uses random.

How was this patch tested?

Unit tests and manual tests

Quick checklist

The following checklist helps ensure your PR is complete:

  • Eclipse Contributor Agreement signed, and signed commits - see IP Requirements page for details
  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.
Signed-off-by: unknown <aboulang2002@yahoo.com>
@aboulang2002 aboulang2002 changed the title RL4J Changed to use Nd4j Random instead of java.util.Random RL4J - Changed to use Nd4j Random instead of java.util.Random Oct 9, 2019
@saudet

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

@AlexDBlack Should we be using Nd4j.getRandomFactory().getNewRandomInstance() for that? I don't see it being used a lot in ND4J or DL4J...

@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

Nd4j.getRandom() put and/or get the instance from a ThreadLocal storage. I was afraid that if Nd4j.getRandom().setSeed(...) is called multiple times, a user might think he is setting the seed on different Random instances but in fact is re-setting the seed on the same instance. This might cause unwanted patterns in the random sequences. This can be solved easily if we just create a new Random with every instance.

On the other hand, I could make sure the seed is only set once and use Nd4j.getRandom(0 everywhere.

@saudet

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

I see, that sounds like a prudent thing to do. I'm wondering if we should be using that factory like that though. I'll let Alex decide.

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

Throughout DL4J and SameDiff, we have tended to use the thread local random (Nd4j.getRandom()) pretty much everywhere, which works well enough most of the time.
The downside of course is that the seed/state is attached to the thread, not to the model. Which may or may not matter.

The problem with using getNewRandomInstance is that anything internally to the network won't be using it that random instance.
So, if you have say a model with dropout, that will continue to use the thread local random. Which means that your model training won't be made deterministic just by creating the new instance with the specified seed. I'm not 100% following what's the intended behavior here when setting the seed, and if that's an issue or not...

Signed-off-by: Alexandre Boulanger <aboulang2002@yahoo.com>
@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

I decided to use Nd4j.getRandom() because it does the job just as well as using the factory and I feel the use of Random in RL4J needs to be similar to the rest of DeepLearning4j. As for my earlier concern that a user calling Nd4j.getRandom().setSeed(mySeed) might unknowingly cause side effects, it's nothing that reading the docs can't clear up.

So these are the specs I implemented:

  • If no seed is set, the learning is completely non-deterministic (i.e. setSeed() should never called)
  • If the seed is set, the learning should be completely deterministic (i.e. every run should always give the same results for a given seed value)
  • The classes that need Random should carry an internal reference. This is easier in the unit tests when we can inject every instance with different mocks that each contains precise 'random' values. (Not all ctors presently support injection of Random but I will add them when required)
@saudet

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@AlexDBlack The thing is, RL4J "uses" DL4J so whatever happens in DL4J w.r.t to seeding comes in too late, and not all threads interact with DL4J either. How do you think we should handle this?

@saudet
saudet approved these changes Oct 15, 2019
Copy link
Member

left a comment

This looks good to me, and I'm getting reproducible results with Cartpole.

@saudet saudet requested a review from AlexDBlack Oct 15, 2019
Copy link
Contributor

left a comment

LGTM 👍

@saudet saudet merged commit 171ce51 into eclipse:master Oct 16, 2019
1 check passed
1 check passed
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.