Skip to content

Conversation

@astroC86
Copy link
Contributor

No description provided.

@astroC86 astroC86 marked this pull request as draft July 14, 2025 23:49
Copy link
Contributor

@mrava87 mrava87 left a 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 MPISummaMatrixMult with another operator like we did for MPIMatrixMult
  • add unittests
  • refractor MatrixMult.py to contain 2 private classes _MPIBlockMatrixMult and _MPISummaMatrixMult and a public class MPIMatrixMult which takes all the inputs required by both of the private classes and a kind parameter to choose between block and summa - see https://github.com/PyLops/pylops/blob/dev/pylops/signalprocessing/fft2d.py for a similar pattern

@astroC86 astroC86 force-pushed the actual-SUMMA branch 6 times, most recently from 21ea369 to a994192 Compare July 26, 2025 19:08
@astroC86 astroC86 requested a review from mrava87 July 27, 2025 14:49
@astroC86 astroC86 marked this pull request as ready for review July 27, 2025 14:50
Copy link

@mrava87SW mrava87SW left a 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=blocked works with CuPy arrays whilst kind=summa leads to a segmentation fault - likely due to the same cause of this #144

@hongyx11
Copy link
Collaborator

hongyx11 commented Sep 3, 2025

@mrava87 I've reviewed the code and think we can merge to the repo

@mrava87
Copy link
Contributor

mrava87 commented Sep 5, 2025

@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

@mrava87 mrava87 mentioned this pull request Sep 9, 2025
@mrava87 mrava87 merged commit 489dfa6 into PyLops:main Sep 9, 2025
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants