Implementation of MPI SUMMA matrix mul #160
Conversation
There was a problem hiding this comment.
@astroC86 very well done, this code looks amazingly clear and (I hope) efficient as the SUMMA promises to be 😄
I think at this point we are left with the following:
- add a chaining of
MPISummaMatrixMultwith another operator like we did forMPIMatrixMult - add unittests
- refractor
MatrixMult.pyto contain 2 private classes_MPIBlockMatrixMultand_MPISummaMatrixMultand a public classMPIMatrixMultwhich takes all the inputs required by both of the private classes and akindparameter to choose betweenblockandsumma- see https://github.com/PyLops/pylops/blob/dev/pylops/signalprocessing/fft2d.py for a similar pattern
| for k in range(self._P_prime): | ||
| Atemp = self.A.copy() if self._col_id == k else np.empty_like(self.A) | ||
| Xtemp = x_block.copy() if self._row_id == k else np.empty_like(x_block) | ||
| self._row_comm.Bcast(Atemp, root=k) |
There was a problem hiding this comment.
question: do you think you could use bcast instead of Bcast?
The reason I am asking is because we currently use always lower-case methods, which although sub-optimal (mpi4py/mpi4py#657) has the advantage of working with CuPy arrays out of the box whilst we saw some issues when @tharittk tried to use upper-case methods (although it was Send/Recv)... my suggestion would be, let's stick to what we currently do and soon (hopefully during @tharittk second part of GSoC) we will switch all MPI4PY calls involving NumPy/CuPy arrays to buffers (aka upper-case methods)
There was a problem hiding this comment.
ok perfect! will do
There was a problem hiding this comment.
super super weird when I changed from Bcast to bcast I am getting wrong values
astroC86
force-pushed
the
actual-SUMMA
branch
6 times, most recently
from
21ea369 to
a994192
Compare
There was a problem hiding this comment.
@astroC86 good job!
I think the code reads well and the way you created the MPIMatrixMult operator. I noticed (even before I pushed some minor stylistic fixes) that one test was failing... but did not have time to look into it, I guess you can probably more quickly figure out why 😉
Once the tests are passing, @hongyx11 has also looked at the SUMMA implementation and we have discussed the point I raised above about upper vs lower case communication routines*, we should be ready to merge this PR!
- as I suspected, after my last commit, the
kind=blockedworks with CuPy arrays whilstkind=summaleads to a segmentation fault - likely due to the same cause of this #144
No description provided.