-
Notifications
You must be signed in to change notification settings - Fork 225
Calculate better seeds #1046
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
Calculate better seeds #1046
Conversation
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.
... (e.g. cell index, time step,...) ...
|
I assigned @ax3l because I am involved at the development process |
|
I am later XORing with the Also: we should also take the number of slides from the moving window into account, right? |
|
We should avoid manipulations of the seed inside the user code like PIConGPU. GlobalSeed globalSeed;
mpi::SeedPerRank<simDim> seedPerRank;
uint32_t seed = seedPerRank(globalSeed(), PMacc::traits::GetUniqueTypeId<FrameType, uint32_t>::uid());
seed ^= POSITION_SEED ^ currentStep;GlobalSeed globalSeed;
uint32_t localShift = currentStep ^
POSITION_SEED ^
PMacc::traits::GetUniqueTypeId<FrameType, uint32_t>::uid();
mpi::SeedPerRank<simDim> seedPerRank;
seed = seedPerRank(globalSeed() , localShift );IMO the second one is the right usage of this pull request. [update] Please see my next post, which suggest to XOR all user variables to one seed and create a MPI unique seed out of it. |
|
Suggestion: IMO the second parameter of |
|
I agree. Maybe one can think of a better name for |
|
Thank you, I am actually eager to review it since it is a quite sensitive part of the code.
It's great you guys investigated it deep and could improve it! Will try to have a look soon.
|
|
I adapted the function to only take 1 param. I also shuffle that param a bit, as it (in all current usages) contains a counted number too (the species-uid) Therefore you easily get collisions there. |
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.
please do not change this algorithm together with the rest of this pull request.
std::numeric_limits<> is not defined for 64bit types in C++98. Please keep the old method.
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.
Ok, reverted.
|
we have to take care that also on a per-time-step basis the seed varies enough. for example, an ionization scheme running on the same cell (position & species information constant) should not create a predictable pattern on each cell over time (nor should that pattern move over the simulation). |
|
I'm afraid this is gonna be impossible. If you only have 32 bits, how could you get that many information into the seed? The current design allows xoring the local seed (got from SeedPerRank) with a counted number without creating collisions (or at least it is unlikely) |
|
it's not about having one unique seed for each cell and time step (which also would not solve that). |
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.
can now be used at each time step, pls add the currentStep xor, too.
|
@ax3l Added the current step |
|
all right, thank you for the great work! ✨ |
- `ionizationMethods.hpp`: - Removed workaround for good seed
after merge of ComputationalRadiationPhysics#1046
- `utilities.hpp`: - Added EOF newline
This improves the global uniqueness of the seeds by using local seeds that vary in the upper bits.
When XORing those with e.g. cellIdxs that are counted (from 0-N) the resulting seeds are still globally unique. Previously the counted rank and the counted cellIdx could cancel each other out resulting in "seed collisions" over different ranks.
Helps e.g. @n01r :)