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

throw away random numbers to avoid unexpected correlations #1569

Merged
merged 9 commits into from Oct 30, 2019

Conversation

@JunChiehWang
Copy link

JunChiehWang commented Jul 18, 2019

Summary

When there is a need to inject particles (with fix deposit), a seed is used to generate a sequence of random numbers for the initial position of particles. There is a strong correlation between the first random number generated by different seeds.

Before the first few random numbers are thrown away, the position of first particle generated by different seed has a strong correlation. The figure shown here is the initial position of the first particles generated by different seeds, it is not uniform.
image

It can be fixed by simply throwing away the first few random numbers generated by each seed.

After the first 30 random numbers are thrown away, the initial position of first particle generated by different seed is uniform:
image

Related Issues

N/A

Author(s)

Jun-Chieh Wang (Applied Materials, wangjj0120@gmail.com)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

By tossing the first 30 random numbers, a given seed will no longer produce exactly the same random numbers to the code/user which breaks the backward compatibility.

Implementation Notes

The first 30 random numbers are thrown away, it's the only thing that has been changed in this file.
image

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted. Delete lines that don't apply

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

Jun-Chieh Wang added 3 commits Jul 18, 2019
@mkanski

This comment has been minimized.

Copy link
Collaborator

mkanski commented Jul 23, 2019

Wouldn't it be better to have this change in the constructor? With the current code, the 30 numbers are discarded for every insertion attempt.

Jun-Chieh Wang Jun-Chieh Wang
@JunChiehWang

This comment has been minimized.

Copy link
Author

JunChiehWang commented Jul 23, 2019

@mkanski It's a good idea! Thank you. I have moved it to the constructor.

src/MISC/fix_deposit.cpp Outdated Show resolved Hide resolved
@JunChiehWang

This comment has been minimized.

Copy link
Author

JunChiehWang commented Jul 24, 2019

@sjplimp I have moved it after the generator is constructed, thanks.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jul 29, 2019

@sjplimp does this need to be expanded to other cases? e.g. fix pour?

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Jul 29, 2019

Sure, the same logic would apply in fix pour. Can't think of any others.

akohlmey added 2 commits Jul 30, 2019
@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jul 30, 2019

Sure, the same logic would apply in fix pour. Can't think of any others.

OK. updated. turned on regression tests, since we may need to reset some of them.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jul 31, 2019

Sure, the same logic would apply in fix pour. Can't think of any others.

what about create_atoms? or fix evaporate? or fix gcmc?

@rdwyman

This comment has been minimized.

Copy link

rdwyman commented Aug 11, 2019

This pull claims it will not break backwards compatibility, though I am afraid it will. Per the Pour documentation, a user provides a seed parameter. The provided seed will lead to a random but repeatable stream of numbers. If I provide the seed 12345 and it produces a stream starting with 5,2,7,8,1..., I would expect the seed 12345 to always produce a stream starting with 5,2,7,8,1.... By tossing the first 30 numbers in the stream, a given seed will no longer produce the same repeatable stream breaking backwards compatibility. This is a minor breakage but might be significant for some users.

@JunChiehWang

This comment has been minimized.

Copy link
Author

JunChiehWang commented Aug 12, 2019

@rdwyman
I agree that for debugging purpose, we do want to produce exactly the same random numbers with the same seeds we provide. Thanks for the comments, I have modified the Backward Compatibility accordingly.

@sjplimp sjplimp assigned akohlmey and unassigned sjplimp Aug 20, 2019
@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Aug 20, 2019

yes, let's also add this 30x warmup to fix evaporate (in the constructor), and to
create_atoms::add_random() in that method. This is the RNG in create_atoms
that is the same for every proc, not the ranmol for the Marsaglia generator.

@akohlmey akohlmey requested review from sjplimp and athomps Aug 22, 2019
@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Aug 22, 2019

@sjplimp added the warmup to two more locations as discussed previously. should be ready to go in now if it passed the integration tests

Copy link
Contributor

sjplimp left a comment

looks good

@akohlmey akohlmey merged commit fed1a72 into lammps:master Oct 30, 2019
8 checks passed
8 checks passed
lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/cmake/cmake-kokkos-cuda-pr head run ended
Details
lammps/pull-requests/cmake/cmake-kokkos-omp-pr head run ended
Details
lammps/pull-requests/cmake/cmake-serial-pr head run ended
Details
lammps/pull-requests/kokkos-omp-pr head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.