Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added support for broadcast in _matvec and _rmatvec in MPILinearOperator #48

Merged
merged 5 commits into from
Jul 21, 2023
Merged

Added support for broadcast in _matvec and _rmatvec in MPILinearOperator #48

merged 5 commits into from
Jul 21, 2023

Conversation

rohanbabbar04
Copy link
Collaborator

Adds #47

  • Added support for broadcast in _matvec and _rmatvec.
  • Added a test to check the above..

@rohanbabbar04
Added support for broadcast and a sample test
7174223 
Minor update

Minor: Rearrange imports

Minor: Remove extra line

Update docstrings
@rohanbabbar04
Copy link
Collaborator Author

rohanbabbar04 commented Jul 20, 2023 •
edited
Loading

@mrava87, wasn't a really big change just needed to add partition=x.partition, I think broadcast support will be really helpful. Also thought about adding a test to check(similar to your example).

  • For single pylops operator we can currently use broadcast x MPIOp @ x.(It just calculates at each rank).
  • For complex operators if we align the shape of MPIOp and Vstack with the x.local_array we can also make the adjoint work with scattered x ...
  • For the broadcast(in your example) it works pretty well and the output returned is as expected(scattered DistributedArray)...

@mrava87
Copy link
Contributor

mrava87 commented Jul 20, 2023 •
edited
Loading

Wow, this is nice. I didn't realize it would have been so easy. And indeed this is going to be a very useful feature.

Just two small comment:

  • I would split the test test_mpilinop into 3 smaller tests, it is usually good practice not to test too many features at the same time.
  • I would also add an example, basically the same or similar code of the # Test Product of MPIVStack and MPILinearOperator with some explanation, to lift this feature up and make users aware of it :D

Apart from these, I am very happy with this PR, well done!

@rohanbabbar04
Copy link
Collaborator Author

rohanbabbar04 commented Jul 20, 2023 •
edited
Loading

@mrava87 Done the changes that were requested

  • Added asmpilinearoperator to docs(we should use this to convert pylops operator to MPILinearOperator)
  • Split the tests into 3 parts.
  • Added an example of MPILinearOperator(Mostly similar, just added plots and some explaination)..

This will close issue #47

mrava87
mrava87 approved these changes Jul 21, 2023
View reviewed changes
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.

Very good, I just made a minor comment on the example. Other than that it is good to go for me :)

examples/plot_mpilinop.py Outdated Show resolved Hide resolved
@rohanbabbar04 rohanbabbar04 linked an issue Jul 21, 2023 that may be closed by this pull request
Add logic to MPILinearOperator to wrap a simple PyLops operator #47
Closed
@rohanbabbar04 rohanbabbar04 merged commit 0c94bfb into main Jul 21, 2023
15 checks passed
@rohanbabbar04 rohanbabbar04 deleted the wrap_broadcast branch July 21, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrava87 mrava87 mrava87 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add logic to MPILinearOperator to wrap a simple PyLops operator
2 participants
@rohanbabbar04 @mrava87