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

SobolEngine should use random seed by default when scrambling #24881

Closed
Balandat opened this issue Aug 20, 2019 · 4 comments
Closed

SobolEngine should use random seed by default when scrambling #24881

Balandat opened this issue Aug 20, 2019 · 4 comments

Comments

@Balandat
Copy link
Contributor

Currently, the following behavior is deterministic:

SobolEngine(dim=d, scramble=True).draw(k)

To achieve random scrambling, you need to do

SobolEngine(dim=d, scramble=True, seed=random.randint(0, 100)).draw(k)

This behavior is inconsistent with that of other random number generation in torch. E.g., torch.rand(d) will generate different numbers in each call, unless the seed is fixed manually.

I believe the SobolEngine behavior should be consistent with this, otherwise this is a pretty big gotcha. This behavior is because if no seed is given to SobolEngine, it uses torch.Generator() to generate the random numbers for scrambling. My suggestion is to use a random seed if seed=None in the SobolEngine constructor.

If this sounds reasonable, I'll put up a two line PR.

@Balandat
Copy link
Contributor Author

@vishwakftw

@vishwakftw
Copy link
Contributor

vishwakftw commented Aug 20, 2019

@Balandat sounds very reasonable. I actually hadn't thought of this at the time. I would be wary of the tests, because they assume the above behavior.

@Balandat
Copy link
Contributor Author

yeah I could have thought of that myself as well at the time when reviewing the PR...

Balandat added a commit to Balandat/pytorch that referenced this issue Aug 20, 2019
facebook-github-bot pushed a commit that referenced this issue Aug 20, 2019
Summary:
Addresses #24881. Makes behavior consistent with the rest of the random functions.
Pull Request resolved: #24884

Test Plan: Unit tests

Reviewed By: sdsingh

Differential Revision: D16912036

Pulled By: Balandat

fbshipit-source-id: eff00cca989926a5d9e20d8846a8674f7cd270cb
@vishwakftw
Copy link
Contributor

Closing since the fix has been merged.

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

No branches or pull requests

2 participants