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

Implementation of HybridDecomposition #4373

Merged
merged 4 commits into from
May 11, 2022

Conversation

pkreissl
Copy link
Contributor

@pkreissl pkreissl commented Oct 20, 2021

Fixes #4351

Description of changes:

  • Implement HybridDecomposition
  • Add tests and doc
  • Make DomainDecomposition class (instead of struct)

@pkreissl pkreissl force-pushed the hybrid_particle_decomposition branch from e184ceb to d9fb26a Compare October 20, 2021 11:12
@RudolfWeeber
Copy link
Contributor

  • Please initialize m_n_square_types to empty set
  • In resort, please make sure the child decompositions don't clear the diff vector
  • in particle to cell you are using the paricle id in the lookup. Should be the particle type, I guess.
  • In the same function, return directly from the if/else branches. No need for a temporary variable.
  • In the constructor: The child decompositions (and HybridDecomposition) currently use move on the communicator. I'm not sure that will work. With move, the original can be rendered unusable. You might need copy semantics
  • You still need to set up the cell neighborships across the two child decompositions. Note that the link cell algorithm (algoriht/linked_cell.hpp) only goes over local_cells and only over red neighbors. So, ghost cells have to be made neighbors of local cells. Probably the easiest solution is to add all cells (local and ghost) of the n_square as red neighbors to the local cells of the domain decomposition.
  • Either you have to make the list of particle types to go into the n_square a consturctor argument of the Hybrid Decomposition and thus immutable, or you have to write a resort, which will move wrongly placed particles between the decompositoins afterwards.

@pkreissl

This comment has been minimized.

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Oct 21, 2021 via email

@pkreissl pkreissl force-pushed the hybrid_particle_decomposition branch from 408bc51 to 925d0ab Compare November 9, 2021 13:27
@pkreissl pkreissl force-pushed the hybrid_particle_decomposition branch from 869f8b5 to 7fb7423 Compare December 17, 2021 11:06
@pkreissl pkreissl force-pushed the hybrid_particle_decomposition branch 2 times, most recently from 063a590 to c853e63 Compare February 4, 2022 09:58
@pkreissl pkreissl force-pushed the hybrid_particle_decomposition branch from 61e78d6 to 3a92913 Compare May 3, 2022 13:28
@pkreissl pkreissl force-pushed the hybrid_particle_decomposition branch from 3a92913 to f0d0533 Compare May 3, 2022 13:58
@pkreissl
Copy link
Contributor Author

pkreissl commented May 3, 2022

Ready for review...

RudolfWeeber
RudolfWeeber previously approved these changes May 10, 2022
Copy link
Contributor

@RudolfWeeber RudolfWeeber left a comment

Choose a reason for hiding this comment

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

JN, could you maybe also tae a look?

Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

I don't have too much expertise in our cell system, but looks good from the technical side

@jngrad jngrad added this to the Espresso 4.2 milestone May 11, 2022
@kodiakhq kodiakhq bot merged commit ad6512f into espressomd:python May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WIP: Introduce hybrid ParticleDecomposition
3 participants