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

UDCT operator #610

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

UDCT operator #610

wants to merge 10 commits into from

Conversation

yud08
Copy link

@yud08 yud08 commented Aug 28, 2024

This is the first working version of udct linear operator. Pass dot test and has basic examples.
First commit., yeah !!!

@mrava87
Copy link
Collaborator

mrava87 commented Sep 2, 2024

@yud08 great stuff! @cako and I will try to take a look soon (just got back from the US, so I may need a week or so to get to it)

Copy link
Collaborator

@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.

Hi @yud08,
good stuff!

I have briefly started to look into this so that I can give you some pointers of things that need improvements for you to start working on whilst I look a bit deeper into the code :)

Apart from the comments that will appear directly in the review, there is one more thing to do with respect to the documentation: add ucurv in doc/source/installation.rst following what we do for all other optional dependencies.

import pylops
plt.close("all")

sz = [512, 512]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should try to write the entire example using the pylops.signalprocessing.UDCT methods (matvec/rmatvec) instead of those in the ucurv library. Of course, you can for example use the ucurv2d_show method but when you compute the forward and backward of your transform you should do this using the pylops operator.

plt.close("all")

sz = [512, 512]
cfg = [[3, 3], [6, 6]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to add some additional text explaining what you are doing, right now it is a bit hard to follow the example


imband = ucurvfwd(img, transform)
plt.figure(figsize=(20, 60))
print(imband.keys())
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this print meant to be there, maybe not whilst you make the plot?


plt.figure()
plt.imshow(np.abs(np.fft.fftshift(np.fft.fftn(err))))
# plt.show()
Copy link
Collaborator

Choose a reason for hiding this comment

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

try to always remove commented codes as these are unlikely useful for our users :)

x = np.random.rand(256 * 256)
y = np.random.rand(262144)
F = pylops.signalprocessing.UDCT(sz, cfg)
print(np.dot(y, F * x))
Copy link
Collaborator

Choose a reason for hiding this comment

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

printing is probably not that useful... but anyways this will likely go away and you will use pylops @/* and .H @ / .H * to perform the forward and adjoint of the curvelet transform instead of the methods of ucurv above

y = np.random.rand(262144)
F = pylops.signalprocessing.UDCT(sz, cfg)
print(np.dot(y, F * x))
print(np.dot(x, F.T * y))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We prefer to use .H @ (or .H *) instead of .T *


The UDCT operator is a wraparound of the ucurvfwd and ucurvinv
calls in the UCURV package. Refer to
https://ucurv.readthedocs.io for a detailed description of the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this already available, so far I get 404?

https://ucurv.readthedocs.io for a detailed description of the
input parameters.

Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have only 3 parameters here, but they should be those of the init method, which are 5 in your case

@@ -0,0 +1,63 @@
import numpy as np
Copy link
Collaborator

Choose a reason for hiding this comment

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

Write all tests using only the pylops method and not the methods of ucurv...

Moreover, the bare minimum tests should have the dottest; in your case since you know the adjoint=inv, you can also add the assert that you have already.

@mrava87 mrava87 changed the title First commit, basic operator and test UDCT operator Oct 4, 2024
@mrava87
Copy link
Collaborator

mrava87 commented Oct 4, 2024

@yud08 just checking you saw my review 😄

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