-
Notifications
You must be signed in to change notification settings - Fork 5
Implementation of MPI SUMMA matrix mul #160
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.
@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
21ea369 to
a994192
Compare
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.
@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
|
@mrava87 I've reviewed the code and think we can merge to the repo |
|
@astroC86 the code has a lot of flake8 issue (https://github.com/PyLops/pylops-mpi/actions/runs/17504191079/job/49724046622?pr=160). Please fix them, and then we can merge this 😄 Note that I pushed a commit to update the documentation, so make sure to pull |
No description provided.