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

FourierRadon2D and FourierRadon3D #611

Merged
merged 15 commits into from
Oct 7, 2024
Merged

Conversation

mrava87
Copy link
Collaborator

@mrava87 mrava87 commented Sep 6, 2024

This PR introduces 2 new operators, namely FourierRadon2D and FourierRadon3D that perform linear/parabolic Radon transform in the frequency domain.

Both of them have 3 backends:

  • numpy: can run with both numpy and cupy arrays, suboptimal performance
  • numba: optimal performance for numpy arrays
  • cuda: optimal performance for cupy arrays (uses a numba-cuda custom kernel for part of the operations in matvec/rmatvec

To test the parabolic transform in 3D, a new utility function pylops.utils.seismicevents.parabolic3d is also added.

The new operators are showcased in plot_fourierradon.py. Moreover, two notebook have been added at https://github.com/PyLops/pylops_notebooks/blob/master/developement/Radon-Fourier.ipynb and https://github.com/PyLops/pylops_notebooks/blob/master/developement-cupy/Radon-Fourier.ipynb

@mrava87 mrava87 changed the base branch from master to dev September 6, 2024 18:09
@mrava87 mrava87 requested a review from cako September 6, 2024 20:09
pylops/utils/seismicevents.py Outdated Show resolved Hide resolved
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.

Left some comments and made a few changes. Unfortunately I'm having an error with the numba engine (if I remove the @jit it works) though. Will try to dig a little deeper, but for now, after the changes, will be good to go.

@mrava87
Copy link
Collaborator Author

mrava87 commented Oct 3, 2024

@cako thanks! I replied to some of your comments, and I am going to act immediately on others.

I am a bit confused about two things:

  • Unfortunately I'm having an error with the numba engine (if I remove the @jit it works) though.: you think this is problem with your machine? And you refer to numba.jit or cuda.jit or both? I tried to re-run those examples on my MacBook pro, Linux workstation, and Colab and I am able to run both numba and cuda codes 🙈
  • I saw you made some changes in the `signalprocessing/init.py file, basically the order of some of the imports changed... and I think this is the cause of all tests failing as I get the same issue locally. But if I bring back the original ordering of imports things get fixed in my local machine and also in the CI. I am curious to know if this new order working for you? And do you get why a different importing order can create this problem (I assume it is probably because in the new order we import a module that requires another module imported later, but I am not so sure myself.. didn't do any test so far)

@cako
Copy link
Collaborator

cako commented Oct 7, 2024

@mrava87 Sorry, you can revert the order of imports back to the original. My stupid ruff reorders the imports automatically and I didnt catch it in the commit. My bad!

I'm not sure what the issue is, I tried different versions of Numba as well. All CPU. If it works on yours, I'm not keen on holding back the PR unless I know what the problem is. I can file a bug if it still doesn't work later.

@mrava87 mrava87 merged commit bcd4da2 into PyLops:dev Oct 7, 2024
13 checks passed
@mrava87 mrava87 deleted the feat-fourierradon branch October 7, 2024 18:49
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