-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix SUNDIALS Context
construction for MPICH
#2678
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.
clang-tidy made some suggestions
c8591c3
to
aa242f8
Compare
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
`MPI_COMM_WORLD` can be int, thus pass the BOUT++ communicator Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
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.
clang-tidy made some suggestions
Do not take the address of the pointer. Compiles with mpich and openmpi
clang-tidy review says "All clean, LGTM! 👍" |
This is needed so we can get a pointer to the communicator, as is needed by SUNContext
pass MPI_Comm* to SUNContext_Create as indicated in the docs https://sundials.readthedocs.io/en/latest/sundials/SUNContext_link.html
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
Did this change run on both MPICH and OpenMPI? If so, it can be merged right now, if not, please can someone test it? I guess this is one case where we definitely do want to test across multiple MPI implementations! |
Yes, I did build it against both MPIs with sundials 6.5.0-1.fc39: With these patches on top of v5.0.0: |
Context
construction for MPICH
Resolves #2677