-
Notifications
You must be signed in to change notification settings - Fork 437
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
Representing qtau- signal attenuation using qtau-dMRI functional basis #1281
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that is a big PR. Unfortunatly, tests do not run on my environment so I made a really quick review.
Anyway, thanks for this PR and I will come back later with more question.
@rutgerfick : I'm excited to see that you have replaced cvxopt with cvxpy! Any chance you could help us make this replacement in the other modules where cvxopt is currently used? See #1180 |
Codecov Report
@@ Coverage Diff @@
## master #1281 +/- ##
==========================================
+ Coverage 87.31% 87.55% +0.23%
==========================================
Files 246 248 +2
Lines 32613 33958 +1345
Branches 3552 3728 +176
==========================================
+ Hits 28477 29732 +1255
- Misses 3275 3337 +62
- Partials 861 889 +28
Continue to review full report at Codecov.
|
1 similar comment
Hi @arokem , it looks like this PR is finally passing Travis and has over 90% coverage on qtdmri.py. I'm leaving for holidays for a couple of weeks now so take your time to review what you want more of this PR. The MEDIA paper that the code is made for should be out soon too, but I can send it to you already if you want some context for this PR? Let me know :-) |
Thanks. Please do send over the paper. Enjoy your holiday!
…On Jul 1, 2017 7:22 AM, "Rutger Fick" ***@***.***> wrote:
Hi @arokem <https://github.com/arokem> , it looks like this PR is finally
passing Travis and has over 90% coverage on qtdmri.py. I'm leaving for
holidays for a couple of weeks now so take your time to review what you
want more of this PR. The MEDIA paper that the code is made for should be
out soon too, but I can send it to you already if you want some context for
this PR? Let me know :-)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1281 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHPNjcHktkIBDi-izZsiFQ09noaAqALks5sJlZBgaJpZM4OH42j>
.
|
1 similar comment
Hi @rutgerfick, it might be good adding a tutorial. You can add it on |
Hi @skoudoro , for the qt-dMRI example I could use a real dMRI data set that I here locally that has multiple orientations, gradient strenghts and diffusion times. I see that in other examples you also use various other data sets. What is your regular process for adding a data set to your data base? or how do you usually go about this? |
Hi @rutgerfick first we need to put the datasets online. We have some space in NITRC or figshare or UW servers (via Ariel Rokem). I can give you access in NITRC to put the files there or you can send the files to @skoudoro and he can upload them. For the second option you will receive an e-mail from Serge with instructions. Our NITRC page is available here Sounds good? |
Hi @Garyfallidis that sounds good. Just send me the instructions, and I'll upload or send the data set to you as soon as I have permission to share it from @demianw and co-authors. |
Question: are any of the data-sets that we are already using potentially
usable for this example? What are the requirements for this?
…On Tue, Aug 8, 2017 at 1:05 PM, Rutger Fick ***@***.***> wrote:
Hi @Garyfallidis <https://github.com/garyfallidis> that sounds good. Just
send me the instructions, and I'll upload or send the data set to you as
soon as I have permission to share it from @demianw
<https://github.com/demianw> and co-authors.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1281 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHPNmyL7dhQx6cxEdc0zj08Q2ufBVWGks5sWL-WgaJpZM4OH42j>
.
|
@arokem , qt-dMRI is specifically designed to analyze multi-shell, multi-diffusion time data sets that have varying gradient directions, gradient strengths AND diffusion times. It follow the recent trend of studying time-dependent features of the diffusion signal by e.g. Novikov and Fieremans. I am not aware of public data sets that have these specifications at this time. |
Of course. That makes sense. Please do make sure that the data can be
released under a CC0 or PPDL license.
…On Tue, Aug 8, 2017 at 1:48 PM, Rutger Fick ***@***.***> wrote:
@arokem <https://github.com/arokem> , qt-dMRI is specifically designed to
analyze multi-shell, multi-diffusion time data sets that have varying
gradient directions, gradient strengths AND diffusion times. It follow the
recent trend of studying time-dependent features of the diffusion signal by
e.g. Novikov and Fieremans.
I am not aware of public data sets that have these specifications at this
time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1281 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHPNrdN0SY2jfd_RDTqiXsZf4ULcxEhks5sWMmCgaJpZM4OH42j>
.
|
1 similar comment
8965d36
to
b5bbde0
Compare
Hi @skoudoro , it seems I've rebased and fixed most of the pep8 errors. The only pep8 issues I haven't fixed were
What do you suggest? |
For the "ambiguous variables", I would use |
@arokem it is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rutgerfick, to improve codecov/patch, you should add test for l1_crossvalidation
function and elastic_crossvalidation
function. I am not sure if they are tested. Furthermore, it seems you have this case which is never tested.
I added a couple of comment too. I hope it helps.
dipy/reconst/tests/test_qtdmri.py
Outdated
qtdmri.QtdmriModel(gtab_4d, radial_order=3) | ||
assert_equal(True, False) | ||
except ValueError: | ||
print('uneven radial_order is caught') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use assert_raises
?
with assert_raises(ValueError):
qtdmri.QtdmriModel(gtab_4d, radial_order=3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
dipy/reconst/tests/test_qtdmri.py
Outdated
qtdmri.QtdmriModel(gtab_4d, radial_order=-1) | ||
assert_equal(True, False) | ||
except ValueError: | ||
print('negative radial_order is caught') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above multiple time. Can you update all try/except below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
dipy/reconst/qtdmri.py
Outdated
try: | ||
lopt = generalized_crossvalidation(data_norm, M, | ||
laplacian_matrix) | ||
except: # noqa: E722 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove # noqa: E722
and replace it by except BaseException:
Can you do it for all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Alright @skoudoro , is it time to merge this bad boy? |
LGTM @rutgerfick. I just need to play a little bit more with the example. If I do not come back to you and if anyone adds any comment by this Sunday (07/28). I will merge it. Thank you |
ok, no other comment, merging. Thanks for the hard work @rutgerfick |
Hello,
This pull request includes the qt-dMRI functional basis representation to simultaneously represent the diffusion signal attenuation over both three-dimensional q-space and diffusion time. It allows for the time-dependent exploration of EAP-based indices and providing smooth interpolation and extrapolation in qtau-space. The MEDIA paper on this work will soon be published, and is originally based on Fick et al. IPMI (2015) and Fick et al. CDMRI (2016).
It can be seen as an extension of the MAP-MRI with a separate temporal basis based on negative exponentials, and code-wise uses many functions from mapmri.py. It also required a minor adaptation to the gradient_table function to take q-values and mapmri.py.
The coefficient estimation can be regularized using either analaytic Laplacian regularization (over the entire 4D signal) or l1-sparsity regularization, or both forming a Graphnet-type regularization. However, at the moment it uses cvxpy to do the optimization (which calls ECOS solver underneath at the moment).
Once the coefficients are estimated, the basis allows for the diffusion time-dependent estimation of known q-space indices (RTOP, RTAP, RTPP, MSD, QIV) and ODF.
I still need to add and correct the testing and include some documentation and examples, but let me know if you would be interested in this addition to dipy, and if you require additional changes :-)
Best,
Dr. Rutger Fick