api/reduce#7
Conversation
- build/ directory - any compiled objects
Current TODOs (in-file) * Unravel how MPI_Datatype operates * Overload for MPI_IN_PLACE? * Testing
…wing overiding of the default master rank within mpilib20_init_finalise.
AlexBuccheri
left a comment
AlexBuccheri
left a comment
There was a problem hiding this comment.
Done the first pass. I guess you'd really like me to hurry up and finish the MPI testing such that you can start writing tests as you go
| !> Variable to receive reduced set | ||
| type(MPI_Datatype), intent(inout) :: recvbuf | ||
| !> MPI Operation | ||
| type(MPI_Op), intent(in) :: operation |
There was a problem hiding this comment.
We don't want to pass the operation. This should be inferable from the input data type.
There was a problem hiding this comment.
How would MPI_MAX and MPI_MIN be infered? Same with MPI_PRODUCT and MPI_SUM?
There was a problem hiding this comment.
Good point. I think we could make operation optional and have default behaviour as sum. My feeling is the majority of use cases are sum. Would also be easy to change.
There was a problem hiding this comment.
- make operation optional and have sum as default.
| !> Wrapping of the MPI_REDUCE methods | ||
| !> TODOs | ||
| !> Unravel how MPI_Datatype operates | ||
| !> Overload for MPI_IN_PLACE? |
There was a problem hiding this comment.
I'm not entirely sure how MPI_IN_PLACE works. For a single communicator, it's a pointless operation. Once they're split, we'd need to consider its use
There was a problem hiding this comment.
MPI_IN_PLACE is actually quite complicated to overload - MPI_IN_PLACE replaces sendbuf for the root process (indicating that the sendbuf and recvbuf are the same) BUT on non-root processes, there needs to be a variable passed via sendbuf. Not sure what this means for overloading for when sendbuf is MPI_IN_PLACE, probably would have MPI_IN_PLACE as an optional logical with logic to ensure correct filling of the (in this case) sendbuf and recvbuf variables for MPI_REDUCE.
https://stackoverflow.com/questions/17741574/in-place-mpi-reduce-crashes-with-openmpi
There was a problem hiding this comment.
Ok, makes sense to ignore in the first instance and add an issue to address. I've never needed to use personally and only seen it recently
- Create issue
| !> Reduces values on all processes to a single value | ||
| !> on process defined by process_id | ||
| !> TODO usage examples | ||
| subroutine mpilib20_reduce_int_scalar(sendbuf, recvbuf, & |
There was a problem hiding this comment.
How do we want to line-split (to keep lines below 90 characters(?)). Here I've split after argument 2 because otherwise one goes over 90, but then operation (potentially being removed soon anyway) and mpi_env are grouped. I've put optional arguments on their own line
There was a problem hiding this comment.
Yeah, not going over 90 (unless a comment) sounds sensible. I often like to put inputs and outputs on different lines but if we retain a consistent arg ordering with MPI commands, then clearly we can't. In which case do whatever looks neat. Not a fan of putting optionals on their own line.
More broadly we should introduce fprettify and set it up how we like, then we can just run it to apply consistent formatting
There was a problem hiding this comment.
- 90 characters sounds good. Do what you think looks readable.
There was a problem hiding this comment.
I'll keep it neat without putting optional arguments on their own line.
Sounds good re fprettify. We should get a config file written for it relatively soon. Once I commit these PR changes I'll have a good and see what get produced.
maxwilliams94
force-pushed
the
api/REDUCE
branch
from
9f4850f to
c25dfad
Compare
- remove generic_template file - useful locally if useful during development - mpilib20_REDUCE interface -> mpilib20_reduce - global imports moved to module scope - recvbuf (receiving buffer) for integer case -> intent(out) - use_rank (integer) renamed to process_id. Persistent rank is then renamed to process in calls. - reduce verbosity when inferring dimension of buffers. Use size(recvbuf) rather than storing in `count` variable. - add assert to check that sendbuf and recvbuf are equal dimension - update todos at module docs (remaining types etc) - update todos at subroutine level: usage examples
maxwilliams94
force-pushed
the
api/REDUCE
branch
from
c25dfad to
ecd0bf7
Compare
2 participants
MPI_REDUCE wrapped by interface mplib20_reduce - basic (probably wrong) implementations for int_scalar and int_vec (rank 1)