-
Notifications
You must be signed in to change notification settings - Fork 2
Feat: Add Wrappers for MPI_AllGatherv #124
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
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.
Same concern I've with this PR as I mentioned in the other PR, i.e. we also need to make sure (by standalone tests) that the way the subroutine is used in pFUnit should be added as a standalone test case.
I see two uses of MPI_Allgather_v
in pFUnit:
call Mpi_allGatherV( &
& values, size(values), MPI_INTEGER, &
& global_list, counts, displacements, MPI_INTEGER, &
& this%mpiCommunicator, ier)
and the other is:
call Mpi_AllgatherV( &
& values_, size(values), MPI_LOGICAL, &
& list, counts, displacements, MPI_LOGICAL, &
& this%mpiCommunicator, ier)
maybe we can ensure that we write test cases for different MPI_types
and/or dimensions of array(s) (if any)
c_sendbuf = c_MPI_IN_PLACE | ||
else | ||
c_sendbuf = c_loc(sendbuf) | ||
end if |
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 we should've a utility function for this now, which should do this work for us now.
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.
Yes we can add utility function like we have for comm_world and other things. Will make a subsequent PR for that.
Thanks for posting it's usecase in pFUnit. I will do this in evening!!(ASAP), for both PRs |
Why don't we do one thing, As you mentioned the use case for them it includes the MPI_LOGICAL and MPI_CHARACTER datatype to be implemented which i would like to handle in separate PR. Get CI fixed -> merge pending PRs -> Add bindings with LOGICAL and CHARACTER -> Add pfUnit like tests for both subroutines (which will invlove to add subroutine which can take character arrays as an input ,currently it takes int/real(8) as input for As doing all in these PRs would clutter things up |
Yes, that's the idea. I just wanted you to be aware that I can't merge PR's for now, and any subsequent PR's might cause merge conflicts, which can be annoying sometimes. |
Now the CI are fixed, Should we merge pending PRs (there are 3 of them ) now? Need your approval |
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.
Looks good to me, thanks @adit4443ya
Towards #121
MPI_Allgatherv is similar to MPI_Gatherv, but instead of gathering data to a single root process, it gathers data to all processes in the communicator.