Conversation
Release v2.3.1
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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]] |
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Is this already available, so far I get 404?
| https://ucurv.readthedocs.io for a detailed description of the | ||
| input parameters. | ||
|
|
||
| Parameters |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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.
|
@yud08 just checking you saw my review 😄 |
2 participants
This is the first working version of udct linear operator. Pass dot test and has basic examples.
First commit., yeah !!!