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.

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

Feature: new Sliding and Patch operators #578

Merged
merged 11 commits into from
May 22, 2024

Conversation

mrava87
Copy link
Collaborator

@mrava87 mrava87 commented May 18, 2024

In this PR, I provide a new implementation for all Sliding and Patch operators that presents the following features:

  • the operators are now directly implemented with _matvec and _rmatvec methods. The old implementation, although elegant in that it relied on using other pylops operators, showed to be inefficient especially in cases with very large number of windows.
  • Two kind of operators Op can be provided: the first applies a single transformation to each window separately (as default from before); the second applies the transformation to all of the windows at the same time.
  • nproc in Sliding3D is deprecated since we do not use anymore HStack/VStack operators that have a multi-threading option. Currently, all of the new implementations are single threaded (although inside they may call numpy methods that are multi-threaded).
  • A new parameter savetaper in added to all operators: if True, all of the tapers are created and saved in a numpy array so that they can be efficiently applied in one go as element-wise multiplication of the numpy arrays (this is fast but can be memory demanding); if False, only unique tapers are saved and applied one by one (this is a bit slower but more memory friendly)

All of the new operators have been benchmarked against the hold one, and an analysis of both the timings and memory usage of the different operators has been performed here: https://github.com/PyLops/pylops_slidingpatching

mrava87 added 10 commits May 8, 2024 22:12
Changed sliding1d from using other operators to being
directly implemented (including also an option to apply
Op simultaneously to all windows).
Changed sliding2d and sliding3d from using other operators to being
directly implemented (including also an option to apply
Op simultaneously to all windows).
Changed patch2d from using other operators to being
directly implemented (including also an option to apply
Op simultaneously to all windows).
Changed patch3d from using other operators to being
directly implemented (including also an option to apply
Op simultaneously to all windows).
@mrava87 mrava87 requested a review from cako May 18, 2024 18:58
@mrava87
Copy link
Collaborator Author

mrava87 commented May 18, 2024

@cako not sure if you have time to take a look at this, any comment/suggestion will be welcome 😄

@cako
Copy link
Collaborator

cako commented May 19, 2024

Will have a look at it! Could you make this repo public: https://github.com/mrava87/pylops_slidingpatching

@mrava87
Copy link
Collaborator Author

mrava87 commented May 19, 2024

Ops, yes :) I also moved it to the PyLops organization

Copy link
Collaborator

@cako cako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few small comments, code looks good. I ran your benchmark repo on CPU, and everything worked as expected. The benchmarks were also followed your numbers pretty closely in which transforms were the fastest. So, LGTM!

pylops/signalprocessing/patch2d.py Outdated Show resolved Hide resolved
pylops/signalprocessing/patch2d.py Show resolved Hide resolved
pylops/signalprocessing/patch2d.py Outdated Show resolved Hide resolved
pylops/signalprocessing/patch2d.py Outdated Show resolved Hide resolved
pylops/signalprocessing/patch3d.py Outdated Show resolved Hide resolved
@mrava87
Copy link
Collaborator Author

mrava87 commented May 20, 2024

@cako thanks a lot for the comments, and great to hear that you also run the benchmarks on your machine with similar results!

I’ll address your comments in the next few days and then merge this PR 😀

@mrava87 mrava87 merged commit 23c5539 into PyLops:dev May 22, 2024
15 checks passed
@mrava87 mrava87 deleted the feature-newslidepatch branch May 22, 2024 18:23
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.

2 participants