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

Merged
merged 76 commits into from Oct 29, 2018

Conversation

Projects
None yet
9 participants
@rutgerfick
Copy link
Contributor

rutgerfick commented Jun 28, 2017

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

@skoudoro
Copy link
Member

skoudoro left a comment

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.

Show resolved Hide resolved dipy/reconst/mapmri.py
Show resolved Hide resolved dipy/reconst/qtdmri.py
Show resolved Hide resolved dipy/reconst/qtdmri.py Outdated
Show resolved Hide resolved dipy/reconst/qtdmri.py Outdated
Show resolved Hide resolved dipy/reconst/qtdmri.py Outdated
Show resolved Hide resolved dipy/reconst/tests/test_qtdmri.py Outdated
Show resolved Hide resolved dipy/reconst/tests/test_qtdmri.py Outdated
Show resolved Hide resolved dipy/reconst/tests/test_qtdmri.py Outdated
Show resolved Hide resolved dipy/reconst/tests/test_qtdmri.py Outdated
Show resolved Hide resolved dipy/reconst/tests/test_qtdmri.py Outdated
@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jun 28, 2017

@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

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Jun 28, 2017

@arokem : As requested I made a pull request for the mapmri module that now uses cvxpy :-) See #1285

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage increased (+3.3%) to 88.765% when pulling 73d463c on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 29, 2017

Codecov Report

Merging #1281 into master will increase coverage by 0.23%.
The diff coverage is 93.38%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
dipy/reconst/mapmri.py 90.95% <100%> (+0.02%) ⬆️
dipy/core/tests/test_gradients.py 98.36% <100%> (+0.24%) ⬆️
dipy/data/fetcher.py 42.36% <12%> (-2.3%) ⬇️
dipy/core/gradients.py 90.51% <82.6%> (-1.6%) ⬇️
dipy/reconst/qtdmri.py 93.94% <93.94%> (ø)
dipy/reconst/tests/test_qtdmri.py 98.93% <98.93%> (ø)
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e28942...56f77f6. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Coverage increased (+3.4%) to 88.857% when pulling 7190f01 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Coverage increased (+3.6%) to 88.98% when pulling 63ad296 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Coverage increased (+3.6%) to 88.98% when pulling 63ad296 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Coverage increased (+3.6%) to 88.98% when pulling 63ad296 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Coverage increased (+3.6%) to 88.98% when pulling bac9d5c on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Coverage increased (+3.6%) to 89.012% when pulling 3be592b on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Coverage increased (+3.6%) to 89.012% when pulling 4523d2e on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 1, 2017

Coverage Status

Coverage increased (+3.6%) to 89.012% when pulling 013ac7e on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 1, 2017

Coverage Status

Coverage decreased (-0.09%) to 85.325% when pulling 2a5e7df on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 1, 2017

Coverage Status

Coverage decreased (-0.09%) to 85.325% when pulling 2a5e7df on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Jul 1, 2017

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 :-)

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jul 1, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 21, 2017

Coverage Status

Coverage decreased (-0.07%) to 85.349% when pulling 4d23da6 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 21, 2017

Coverage Status

Coverage decreased (-0.07%) to 85.349% when pulling 4d23da6 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 28, 2017

Hi @rutgerfick, it might be good adding a tutorial. You can add it on doc/examples folder. For sure, advanced user will read your paper, but the other one need a guidance. Thank you for doing this

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Aug 5, 2017

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?

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Aug 8, 2017

Hi @rutgerfick first we need to put the datasets online. We have some space in NITRC or figshare or UW servers (via Ariel Rokem).
Then after you have your files online you will need to create a fetcher/reader function in dipy/data/fetcher.py See for example line 258 and 468 of fetcher.py.

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
https://www.nitrc.org/projects/dipy/

Sounds good?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage increased (+0.4%) to 85.784% when pulling 6f505db on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Aug 8, 2017

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.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 8, 2017

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Aug 8, 2017

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

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Aug 8, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage increased (+0.4%) to 85.784% when pulling bb41789 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage increased (+0.4%) to 85.784% when pulling bb41789 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage increased (+0.4%) to 85.817% when pulling bb41789 on AthenaEPI:athena_qtdmri into 5136340 on nipy:master.

@rutgerfick rutgerfick force-pushed the AthenaEPI:athena_qtdmri branch from 8965d36 to b5bbde0 Oct 22, 2018

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Oct 22, 2018

Hi @skoudoro , it seems I've rebased and fixed most of the pep8 errors.

The only pep8 issues I haven't fixed were

  • the "ambigious name" ones where the variable name is for example lower-case L or upper-case i. They are chosen to be consistent with the paper.
  • the bare excepts for when a voxel fails to fit for whatever reason (cvxpy or anything) that then pass a boolean to the fitted model whether or not the fitting was a success for that voxel. I suppress the QA for these few lines.

What do you suggest?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Oct 22, 2018

For the "ambiguous variables", I would use ll or ii (double the letter). Makes it much easier to search for the variable in a long file (for example)

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Oct 22, 2018

@arokem it is done.

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Oct 23, 2018

Hi @skoudoro @arokem , seems everything passed except for codecov/patch. Can you advise?

@skoudoro
Copy link
Member

skoudoro left a comment

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.

qtdmri.QtdmriModel(gtab_4d, radial_order=3)
assert_equal(True, False)
except ValueError:
print('uneven radial_order is caught')

This comment has been minimized.

@skoudoro

skoudoro Oct 23, 2018

Member

Can you use assert_raises?

with assert_raises(ValueError):
    qtdmri.QtdmriModel(gtab_4d, radial_order=3)

This comment has been minimized.

@rutgerfick

rutgerfick Oct 23, 2018

Contributor

Changed.

qtdmri.QtdmriModel(gtab_4d, radial_order=-1)
assert_equal(True, False)
except ValueError:
print('negative radial_order is caught')

This comment has been minimized.

@skoudoro

skoudoro Oct 23, 2018

Member

same as above multiple time. Can you update all try/except below?

This comment has been minimized.

@rutgerfick

rutgerfick Oct 23, 2018

Contributor

Changed.

try:
lopt = generalized_crossvalidation(data_norm, M,
laplacian_matrix)
except: # noqa: E722

This comment has been minimized.

@skoudoro

skoudoro Oct 23, 2018

Member

Can you remove # noqa: E722and replace it by except BaseException:

Can you do it for all of them?

This comment has been minimized.

@rutgerfick

rutgerfick Oct 23, 2018

Contributor

Changed.

@rutgerfick

This comment has been minimized.

Copy link
Contributor

rutgerfick commented Oct 23, 2018

Alright @skoudoro , is it time to merge this bad boy?

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Oct 23, 2018

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

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Oct 29, 2018

ok, no other comment, merging. Thanks for the hard work @rutgerfick

@skoudoro skoudoro merged commit c0daaeb into nipy:master Oct 29, 2018

4 checks passed

codecov/patch 93.38% of diff hit (target 87.31%)
Details
codecov/project 87.55% (+0.23%) compared to f887b77
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rutgerfick rutgerfick deleted the AthenaEPI:athena_qtdmri branch Oct 29, 2018

@rutgerfick rutgerfick restored the AthenaEPI:athena_qtdmri branch Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment