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

Speedup particle sorting #1167

Merged
merged 3 commits into from Aug 16, 2016

Conversation

gassmoeller
Copy link
Member

Particles now also store their location in the
coordinate system of their current cell. This decreases the
number of times this location has to be computed by inverting
the mapping for the current cell, which is expensive.
On average this change will save 40-50% of the overall
particle computing time, while increasing the particle
memory footprint (which is usually small compared to the
system matrix).

The new algorithm relies on the fact that the reference location is set before the particles are advected. All implemented particle generators do that, but it is possible to use the old constructor to construct a particle without setting this location (e.g. in a user-written generator). What should I do about the old constructor? Remove? Deprecate? Add a sentence to the documentation that if using this constructor the user has to set the reference_location manually?

*/
Particle (const Point<dim> &new_location,
const Point<dim> &new_reference_location,
const types::particle_index &new_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't particle_index just some plain old integer? If it's a scalar, just passing it by const value is fine, no need for a reference.

@bangerth
Copy link
Contributor

I must admit that I haven't really tried to understand the details of the algorithm, but it looks good to me with minor edits.

Regarding data structure: If I understood this right, you are using the list because you need to delete elements from it, right? You could alternatively keep two vectors, with the second already sized to the size of the first but initially empty (via std::vector::reserve), and instead of deleting elements you don't want, you just copy those that you do want to keep. This may be the faster algorithm. Whether it makes a difference, I don't know.

Constructor: Just remove the old constructor.

@gassmoeller gassmoeller force-pushed the speedup_particle_sorting branch 2 times, most recently from a61c6eb to de24eb5 Compare August 15, 2016 17:20
@gassmoeller
Copy link
Member Author

Turns out even in this situation the vector is a few percent faster. I changed everything, and when the tester is happy this should be ready to merge.

*
* @param [in,out] particles_to_sort Vector containing all pairs of
* particles and their old cells that will be sorted into the
* 'particles' member variable in this function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [in,out] can't quite be true any more because the argument is const.

@bangerth
Copy link
Contributor

Looks good. Please address the final few comments and merge yourself.

@gassmoeller gassmoeller merged commit dee3cd0 into geodynamics:master Aug 16, 2016
@gassmoeller gassmoeller deleted the speedup_particle_sorting branch August 16, 2016 19:07
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

Successfully merging this pull request may close these issues.

None yet

2 participants