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

Add MRI Examples #111

Merged
merged 1 commit into from
Oct 18, 2019
Merged

Add MRI Examples #111

merged 1 commit into from
Oct 18, 2019

Conversation

chaithyagr
Copy link
Contributor

No description provided.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 291

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 65.052%

Totals Coverage Status
Change from base Build 289: 0.0%
Covered Lines: 2524
Relevant Lines: 3880

💛 - Coveralls

@sfarrens
Copy link
Contributor

@chaithyagr Have you tested the non_cartesian_reconstruction.py example using Python 3.7? I could only make pynfft work with Python 3.6.

@chaithyagr
Copy link
Contributor Author

@chaithyagr Have you tested the non_cartesian_reconstruction.py example using Python 3.7? I could only make pynfft work with Python 3.6.

I am not sure, let me do that once. If I remember right, we need to use pynfft from github: pip install git+https://github.com/ghisvail/pyNFFT.git
Can you please let me know that you still are facing issues with it?

@sfarrens
Copy link
Contributor

I didn't save the output, but the latest PyPi version of pynfft did not compile with Python 3.7. Some Cython issue if I remember correctly.

@chaithyagr
Copy link
Contributor Author

I think I faced same issue, the latest PyPi version is not good with some version of pythons and thus we rely on the github version. We could request a new release if needed from ghisvail

@sfarrens
Copy link
Contributor

For the moment, I think we should just include a check in the example to see if pynfft is installed, if not raise a warning and exit normally.

@philouc
Copy link
Contributor

philouc commented Oct 16, 2019

Note that Jyh-Miin Lin will arrive at CEA Grenoble in Oct 21. This should ease the interactions. Once he joins us, I will invite him to slack and open pysap access for him as contributor if everyone agrees on that?

@chaithyagr
Copy link
Contributor Author

@philouc note that this is pynfft and not pyNUFFT, we have ongoing PR for best working with pyNUFFT, and surely we will have a round of discussion on that. But for now, we are using NFFT which is developed by Ghishval and which is just a wrapper to nfft3 C library.

@chaithyagr
Copy link
Contributor Author

chaithyagr commented Oct 16, 2019

For the moment, I think we should just include a check in the example to see if pynfft is installed, if not raise a warning and exit normally.

This is exactly what we do in pysap-mri :
https://github.com/CEA-COSMIC/pysap-mri/blob/master/mri/reconstruct/fourier.py#L23-L28

@sfarrens sfarrens merged commit 5f4b509 into CEA-COSMIC:master Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants