Improve statistical particle generation #1127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| for (types::particle_index current_particle_index = 0; current_particle_index < n_local_particles; ++current_particle_index) | ||
| { | ||
| // Draw the random number that determines the cell of the particle | ||
| const double random_weight = accumulated_cell_weights.back() * uniform_distribution_01(this->random_number_generator); |
There was a problem hiding this comment.
break this line.
Is there a way to just generate a random number in the range [0,accumulated_cell_weights.back()]? This would avoid the multiplication every time. (Not that it's going to make a runtime difference, the multiplication has to happen somewhere. I think it may just be simpler to read.)
|
Yes, delightfully short and simple! See if you can do something with the one comment I had, and then merge yourself (assuming the tester is happy). Thanks! |
|
Can you squash this into self-contained commits (it's a sequence of fix-ups right now :-)? There are currently two tests that fail, but they are unrelated to your work. Please merge after squashing. |
gassmoeller
force-pushed
the
improve_statistical_particle_generation
branch
from
9eaadb2
to
68d32a9
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@bangerth: These are some of the changes we talked about on Tuesday, and they partly adress #1035. For a model on my laptop with 1 process, 256 cells, and 500,000 particles and Q1 mapping it saves approximately 40% of the particle generation time (0.45 -> 0.25 seconds), but that will of course change with different number of cells, different geometries, and so on.
Sadly all tests change, because the random number generator is called in a different order (now it first selects cells for all particles, then selects positions for all particles, instead of alternating between the two).
It is hard to see in the test results if the statistical distributions are still correct, but upon visual inspection they look ok (on 1 process and also using multiple processes).
A nice visual side effect of the change is that particle ids at model start are now more regular, because they also increase cell-by-cell, not only process-by-process as before.
In principle this change also allows the more complex creation of particles on the reference cell as we discussed, but since I compute the cell_weight as cell-wise-constant at the moment, it would not be more accurate, it would only save some more time unless we change that as well. Let's first make a note that this is now theoretically possible.