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

Cleanup MPI calls #278

Merged
merged 17 commits into from
Jul 19, 2024
Merged

Cleanup MPI calls #278

merged 17 commits into from
Jul 19, 2024

Conversation

ldowen
Copy link
Collaborator

@ldowen ldowen commented Jun 26, 2024

Summary

This PR improves the interface to the MPI functionality.

  • This PR is a refactoring and bugfix
  • It does the following:
    • Refactors the allReduce and scan wrappers to use the Communicator::communicator() by default and puts both in a single file.
    • Refactors the Distributed source directory, having it provide the MPI wrappers even when MPI is not enabled; this prevents potential bugs from occurring when wrappers like Communicator are present but Communicator.hh is not available.
    • Replaces the MPI_SUM, MPI_MAX, etc with SPHERAL_OP_SUM, SPHERAL_OP_MAX etc

ToDo :

  • Annotate RELEASE_NOTES.md with notable changes.
  • Create LLNLSpheral PR pointing at this branch. (PR#88)
  • LLNLSpheral PR has passed all tests.

…put for allReduce, moved some MPI functions from Utilities to Distributed, combined scan.hh with allReduce.hh and removed mpiUtilities
@ldowen ldowen mentioned this pull request Jun 28, 2024
@mdavis36
Copy link
Collaborator

mdavis36 commented Jul 2, 2024

What do you guys think about using something like SPHERAL_OP_MIN, SPHERAL_OP_MAX etc. Instead of SPHERAL_MPI_MIN, SPHERAL_MPI_MAX. It feels like those macros suggest an MPI call instead of just defining an execution context agnostic operator.

@ldowen
Copy link
Collaborator Author

ldowen commented Jul 2, 2024

Yeah I see what you mean. That works for me. I will change it now.

src/Distributed/allReduce.hh Outdated Show resolved Hide resolved
src/FractalStruct/check_for_edge_trouble.cc Outdated Show resolved Hide resolved
src/KernelIntegrator/FlatConnectivity.cc Outdated Show resolved Hide resolved
mdavis36
mdavis36 previously approved these changes Jul 18, 2024
@ldowen ldowen self-assigned this Jul 19, 2024
@ldowen ldowen merged commit 22f4c5f into develop Jul 19, 2024
1 check passed
@ldowen ldowen deleted the bugfix/central_mpi_calls branch July 30, 2024 16:50
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.

3 participants