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

Particle-grid periodic communication #263

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

streeve
Copy link
Member

@streeve streeve commented Jun 26, 2020

This adds the first direct particle-grid capability in Cabana within core, with dependency on Cajita. This is all in particle_grid/ for now, but I'm open to any reorganization and renaming. CabanaPG directly relies on Core and Cajita and is enabled by default if Cajita is.

This wraps calls to Distributor, migrate, Halo, and gather to simplify things for any user with periodic boundaries or load balancing needs.

New options include:

  • gridDistributor (stolen from ExaMPM), determining new ranks destinations and wrapping through periodic boundaries
  • gridMigrate using gridDistributor and calling migrate
  • migrate tests
  • gridHalo determining particles to be ghosted and any necessary periodic shifts
  • PeriodicHalo inheriting from Halo with functor to add periodic shifts
  • PeriodicShift functor to modify buffer (cannot inherit from Halo because CommPlan contains std member variables, an issue for NVCC)
  • gridGather using PeriodicShift and gridHalo and calling gather
  • Update to gather for modifying the send buffer with periodic shifts
  • halo tests

Closes #77

@streeve streeve added the enhancement New feature or request label Jun 26, 2020
@streeve streeve requested a review from sslattery June 26, 2020 19:08
Copy link
Collaborator

@sslattery sslattery left a comment

Choose a reason for hiding this comment

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

We should discuss but my thoughts are that this should be in core. We should also discuss whether or not to include the halo region at this point in the communication. I think we could have one general cartesian communication structure (both for halo and migration) which incorporates the halo information if needed. We could do this now or treat this as a good start and do more PRs to generalize it.

particle_grid/src/Cabana_Periodic.hpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
particle_grid/src/Cabana_Periodic.hpp Outdated Show resolved Hide resolved

// Create a grid local_grid.
auto local_grid = Cajita::createLocalGrid( global_grid, 1 );
auto local_mesh = Cajita::createLocalMesh<TEST_DEVICE>( *local_grid );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it seem reasonable to have to make all of these objects for cases like MD with short-range only where you don't need a mesh? I could see automating how often migrate is called based on setting some halo in MD. That would then eliminate the need to tune how often migrate happens (e.g. every 100 time steps or something). It would instead be based on some distance criterion which would correspond to a halo width. In that case you would want to create all of these objects then as your cell size would have some distance relative to something meaningful in the simulation (like the interaction distance).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note this extra grid we would use to choose communications would also be the same grid from which we would build LinkedCellList objects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And now that I think about it we need a LinkedCellList that uses Cajita instead of the grid we have as an implementation detail. Even more reason for all of this stuff to just be in core.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I'm not sure how to get around creating all these objects, but I have considered specializations of the mesh/grid that don't hold as much information since we basically only need the bounds of the box in MD

And since we have to rebuild the neighbor list every so often, we do the same with migration, but you're right that it could potentially be done automatically with a check for anything crossing halo boundaries.

I have the LinkedCellList on my list after the PeriodicHalo

@streeve
Copy link
Member Author

streeve commented Jun 26, 2020

I think I addressed everything except for how to potentially generalize for other particle-grid methods. Now that I did it as you suggested, this makes much more sense than a separate particle-grid section

@streeve streeve marked this pull request as draft July 2, 2020 19:57
@streeve streeve changed the title Particle-grid periodic migrate Particle-grid periodic communication Jul 2, 2020
@streeve streeve force-pushed the periodic_migrate branch 2 times, most recently from 64db1ca to fc4eb4e Compare July 9, 2020 19:32
@streeve streeve changed the title Particle-grid periodic communication WIP: Particle-grid periodic communication Jul 16, 2020
@streeve streeve force-pushed the periodic_migrate branch 2 times, most recently from 73e13cb to 73966be Compare August 6, 2020 03:13
@streeve streeve force-pushed the periodic_migrate branch 2 times, most recently from 6efa3c4 to e724c72 Compare August 8, 2020 00:14
@junghans
Copy link
Member

junghans commented Sep 3, 2020

@streeve that helped with the build, but now the Cabana_ParticleGridCommunication_test_OPENMP_1 test fails.

@streeve
Copy link
Member Author

streeve commented Sep 3, 2020

@streeve that helped with the build, but now the Cabana_ParticleGridCommunication_test_OPENMP_1 test fails.

Thanks - that's expected at the moment. gridMigrate passes everything, but gridGather has some more issues to fix.

@streeve
Copy link
Member Author

streeve commented Sep 11, 2020

This will fail until rebased with 295 in master; after that, probably a few more fixes to pass halo tests and it will be ready for review.

@sslattery
Copy link
Collaborator

#295 has been merged - let's proceed with this one and see if it resolves our comm seg faults.

@streeve streeve marked this pull request as ready for review September 24, 2020 00:17
@streeve streeve closed this Sep 24, 2020
@streeve streeve reopened this Sep 24, 2020
@sslattery
Copy link
Collaborator

Let's rebase this one to get the latest changes for the number of testing ranks and see what the CI does. We likely still have a UVM error though.

@streeve streeve changed the title WIP: Particle-grid periodic communication Particle-grid periodic communication Dec 17, 2020
@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #263 (0a67b77) into master (d46804f) will decrease coverage by 0.2%.
The diff coverage is 94.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #263     +/-   ##
========================================
- Coverage    93.5%   93.3%   -0.3%     
========================================
  Files          37      38      +1     
  Lines        2797    2842     +45     
========================================
+ Hits         2617    2653     +36     
- Misses        180     189      +9     
Flag Coverage Δ
clang 93.3% <94.9%> (-0.3%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
core/src/Cabana_CommunicationPlan.hpp 0.0% <0.0%> (-98.3%) ⬇️
core/src/Cabana_Halo.hpp 96.1% <95.6%> (+0.3%) ⬆️
core/src/Cabana_ParticleGridCommunication.hpp 99.1% <99.1%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d46804f...afb82ac. Read the comment docs.

@streeve streeve force-pushed the periodic_migrate branch 2 times, most recently from 6b0827e to 05f3ca3 Compare December 30, 2020 19:29
@streeve
Copy link
Member Author

streeve commented Jan 4, 2021

@sslattery this is ready for another review. @junghans codecov is telling me I reduced coverage for CommPlan to zero - any ideas?

*/
template <class LocalGridType, class PositionSliceType>
Distributor<typename PositionSliceType::device_type>
gridDistributor( const LocalGridType& local_grid, PositionSliceType& positions )
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer this be named createGridDistributor to indicate the creation mechanism.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

*/
template <class LocalGridType, class PositionSliceType,
std::size_t PositionIndex>
auto gridHalo(
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should also be named createGridHalo

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

// Create the Shifts.
auto periodic_shift = PeriodicShift<device_type, PositionIndex>( shifts );

return std::make_pair( halo, periodic_shift );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a pair we should just have a new struct which holds a Halo and the functor the user wants to apply. These would eliminate a user needed to store multiple objects and think about using the pair. In addition, it would allow the user to have an object they could easily append other modification functions if they wanted to for some reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done - although I'm not sure if it's composed to be extensible in the way you mentioned

auto center_z = width_z + lo_z;
auto shift_x = width_x - ( halo_width - 0.1 ) * dx;
auto shift_y = width_y - ( halo_width - 0.1 ) * dy;
auto shift_z = width_z - ( halo_width - 0.1 ) * dz;
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of the setup code looks the same for both tests - perhaps it can be combined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Combined

Cabana::AoSoA<DataTypes, TEST_MEMSPACE> data_src( "data_src", num_data );
Cabana::deep_copy( data_src, data_host );

if ( test_type == 0 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you anticipate adding other values for test_type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a slice variant of gridGather for a second test_type

bool within_y = true;
bool within_z = true;
// Make sure all ghosts are in halo region in at least one direction.
if ( pos_host( i, Cajita::Dim::I ) < lo_x )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a particle field which contains the original MPI rank and then use that to back-calculate what the position should be exactly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MPI support for Periodic Boundaries and Cartesian Decompositions
3 participants