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

[SMALL] Add support for Undecimated Wavelet Transform, with right adjoint operator #38

Merged
merged 9 commits into from
Oct 3, 2019

Conversation

chaithyagr
Copy link
Contributor

This is a minor PR that will hold the undecimated Wavelet transform class for 2D, which pulls in the transform filters from Sparse2D with help of Modopt.

This is a temporary solution while we wait for general solution from CEA-COSMIC/pysap#106

This PR is particularly important to get the first merge for #35 with cartesian and non-cartesian reconstructions.

Copy link
Contributor

@zaccharieramzi zaccharieramzi left a comment

Choose a reason for hiding this comment

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

I think the op and adj_op code need to be refactored. A lot of code is used in common between the multichannel and single channel cases.
For example a op_single_channel and a adj_op_single_channel could be implemented.

mri/reconstruct/linear.py Outdated Show resolved Hide resolved
mri/reconstruct/linear.py Show resolved Hide resolved
mri/reconstruct/linear.py Outdated Show resolved Hide resolved
mri/test/test_wavelet_adjoint.py Outdated Show resolved Hide resolved
@chaithyagr
Copy link
Contributor Author

Adding, I see that Travis Build also has failed due to missing Sparse2D installations. I will try to get this fixed here as we may need this in the future also.
Note that going with ModOpt based way to obtain MR Filters, we will face this issue, the installations and environment variables must be rightly set.

@zaccharieramzi
Copy link
Contributor

Yes that will have to be explained in the docs somewhere too.
The fix is something like the following:
export PATH=$PATH:/path-to-pysap/build/temp.linux-x86_64-3.6/extern/bin/

@chaithyagr chaithyagr requested a review from AGrigis September 30, 2019 13:16
@chaithyagr
Copy link
Contributor Author

Adding @AGrigis to review the travis code updates.

@AGrigis :
Note that this line is temporary:
- pip install git+https://github.com/chaithyagr/ModOpt.git

We are awaiting CEA-COSMIC/ModOpt#64 to be merged to mainline so that we can directly point to CEA_COSMIC master branch.

Copy link
Contributor

@zaccharieramzi zaccharieramzi left a comment

Choose a reason for hiding this comment

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

Some questions especially regarding the parallel execution of the wavelet op.

mri/reconstruct/linear.py Show resolved Hide resolved
mri/reconstruct/linear.py Show resolved Hide resolved
mri/reconstruct/linear.py Outdated Show resolved Hide resolved
mri/reconstruct/linear.py Outdated Show resolved Hide resolved
mri/test/test_wavelet_adjoint.py Show resolved Hide resolved
mri/test/test_wavelet_adjoint.py Outdated Show resolved Hide resolved
mri/reconstruct/linear.py Show resolved Hide resolved
Add backend support for multichannel parallel run
Update docs for verbosity level
Copy link
Contributor

@zaccharieramzi zaccharieramzi left a comment

Choose a reason for hiding this comment

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

LGTM, maybe let's wait for @AGrigis 's input for the travis part, but I am not too worried as this is a hot-fix

@chaithyagr
Copy link
Contributor Author

Yes, I'll let @LElgueddari just give a quick look at it. Ill mostly talk off-hand with @AGrigis about this.
More importantly right now to get this in, we would need CEA-COSMIC/ModOpt#64 to be in so that I could point to master branch than my github repository

@AGrigis
Copy link
Contributor

AGrigis commented Oct 1, 2019

I suggest to test against the master banch of pysap and modopt but also from the released version available on pypi.
It can avoid issues with the end user that will probably mix everything.

@chaithyagr
Copy link
Contributor Author

I suggest to test against the master banch of pysap and modopt but also from the released version available on pypi.
It can avoid issues with the end user that will probably mix everything.

I agree on this, we could use matrix to test with both cases. However, we would need pysap to export the sparse2D binaries to a location for this, (perhaps in /.local/pysap/bin , so that we could do a pip install and point the PATH variable.
I will open an issue in pysap-mri to update travis, and one in PySAP to ensure we export the binaries.

@chaithyagr
Copy link
Contributor Author

All codes are in. Travis updated to point to CEA-COSMIC master branch of Modopt.
Last checks by @LElgueddari and we can get this to mainline..

@chaithyagr
Copy link
Contributor Author

Had an offline chat with @LElgueddari , we plan to merge this as there isnt any specific feedback we may require from Loubna for this merge.

Merging to Master

@chaithyagr chaithyagr merged commit 1cc2da9 into CEA-COSMIC:master Oct 3, 2019
@chaithyagr chaithyagr deleted the examples branch October 4, 2019 09:04
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.

3 participants