-
Notifications
You must be signed in to change notification settings - Fork 10
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
Update for MPI_PARALLEL flag #203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
include/specfem_mpi/specfem_mpi.hpp
Outdated
@@ -18,7 +18,10 @@ namespace MPI { | |||
* Incase specfem is compiled without MPI then I need placeholders for reducer | |||
* types | |||
*/ | |||
enum reduce_type { sum = MPI_SUM, min = MPI_MIN, max = MPI_MAX }; | |||
using reduce_type = MPI_Op; | |||
const MPI_Op sum = MPI_SUM; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'd need constexpr static MPI_Op
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the compilation will fail when MPI is enabled if tis a const MPI_OP
itd have it be a static variable. Constexpr since we already know it at compile time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned comments earlier
retest please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
retest please |
Retest this please |
@icui the tests won’t rerun on this. Since it’s being merged into a ‘issue-*’ branch. You should merge this PR if ready |
Description
Please describe the changes/features in this pull request.
Issue Number
If there is an issue created for these changes, link it here
Checklist
Please make sure to check developer documentation on specfem docs.
[] I ran the code through pre-commit to check style
[] My code passes all the integration tests
[] I have added sufficient unittests to test my changes
[] I have added/updated documentation for the changes I am proposing
[] I have updated CMakeLists to ensure my code builds
[] My code builds across all platforms