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

WIP: Add mapmri flow #1350

Closed
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@aarya22
Contributor

aarya22 commented Oct 7, 2017

Not sure how to create a workflow or if this is correct

@Garyfallidis could you check this out and see if this is what you meant?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 8, 2017

Hi @Aarya, okay, you are moving towards the correct direction but not there yet. Will start commenting asap. Thanks!

@aarya22

This comment has been minimized.

Contributor

aarya22 commented Oct 11, 2017

@Garyfallidis anything I can change?

@skoudoro

Hi @aarya22, it is a good start! I made some comment on your PR. Firstfall, you should do a git rebase on your code to update your code with the mainstream. Second, build test failed because of your matplotlib dependency (you can see it here. I suppose you do not really need this dependency. You can read the comment below to see why.
Let me know if you have any question or if you need help.

Thanks for this contribution!

from dipy.reconst import mapmri
import matplotlib
matplotlib.use('agg')
import matplotlib.pyplot as plt

This comment has been minimized.

@skoudoro

skoudoro Oct 12, 2017

Member

I think, you do not need matplotlib here

def get_short_name(cls):
return 'mapmri'
def run(self, data_file, data_bvecs, data_bvals, small_delta, big_delta):

This comment has been minimized.

@skoudoro

skoudoro Oct 12, 2017

Member

you should define output files init the run function. Look at the example on the following link : https://github.com/nipy/dipy/blob/master/dipy/workflows/reconst.py#L34

It will be good if you can add default value too. Indeed, it will be useful for beginners who do not know what to put.

return 'mapmri'
def run(self, data_file, data_bvecs, data_bvals, small_delta, big_delta):
img = nib.load(data_file)

This comment has been minimized.

@skoudoro

skoudoro Oct 12, 2017

Member

try to use io_iteratorlike the example here. It permits you to read many input files.

def run(self, data_file, data_bvecs, data_bvals, small_delta, big_delta):
img = nib.load(data_file)
bvals,bvecs = read_bvals_bvecs(data_bval,data_bvec)

This comment has been minimized.

@skoudoro

skoudoro Oct 12, 2017

Member

same as above

cax = divider.append_axes("right", size="5%", pad=0.05)
plt.colorbar(ind, cax=cax)
plt.savefig('MAPMRI_maps_regularization.png')

This comment has been minimized.

@skoudoro

skoudoro Oct 12, 2017

Member

I suppose we do not want to save this data in this way but we prefer to return many output files that can be input files for another process (or workflow). Currently, we can not do that with your code. So you should remove the matplolib part and replace it. you can see an example in the link above

return 'mapmri'
def run(self, data_file, data_bvecs, data_bvals, small_delta, big_delta):
img = nib.load(data_file)

This comment has been minimized.

@skoudoro

skoudoro Oct 12, 2017

Member

Documentation is really important here. You can look at the example here how to make a good documentation

@codecov-io

This comment has been minimized.

codecov-io commented Oct 31, 2017

Codecov Report

Merging #1350 into master will decrease coverage by <.01%.
The diff coverage is 83.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1350      +/-   ##
==========================================
- Coverage   87.07%   87.06%   -0.01%     
==========================================
  Files         227      228       +1     
  Lines       29060    29170     +110     
  Branches     3123     3136      +13     
==========================================
+ Hits        25303    25398      +95     
- Misses       3047     3051       +4     
- Partials      710      721      +11
Impacted Files Coverage Δ
dipy/workflows/tests/test_reconst_dti.py 95.08% <ø> (ø) ⬆️
dipy/workflows/reconst.py 79.38% <79.41%> (-0.12%) ⬇️
dipy/workflows/tests/test_reconst_mapmri.py 90.69% <90.69%> (ø)
dipy/reconst/mapmri.py 89.78% <0%> (+0.38%) ⬆️

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 e34445c...af4cd7d. Read the comment docs.

@aarya22

This comment has been minimized.

Contributor

aarya22 commented Oct 31, 2017

Sorry for the long delay in replying to your comments @skoudoro I have been busy with school and midterms. Take a look at the updated workflow. Unfortunately, I cannot change the output of the workflow. Brain-Life is planning on using this workflow to power one of its applications (app-dipy-mapmri) and needs the png file as its output in order to work. I am also not sure what else to change it to so it can be compatible with other dipy workflows. Let me know if there are any problems here and hopefully I can change it promptly.

@skoudoro

no problem and thanks for that @aarya22! I made some few comments below.

Can you add a test for your workflow? you will find an example in dipy/workflows/tests/test_reconst_dti

I am surprised that this workflow output is a plot screenshot and I wonder How you can use it as an input of other brain-life apps? Can you explain me more?

Path to the input volume.
data_bvecs : string
Path to the bvec files.
data_bvals :

This comment has been minimized.

@skoudoro

skoudoro Oct 31, 2017

Member

add the datatype -> data_bvals : string

Path to the bvec files.
data_bvals :
Path to the bval files.
small_delta :

This comment has been minimized.

@skoudoro

skoudoro Oct 31, 2017

Member

add the datatype -> small_delta : float

small_delta :
Small delta value used in generation of gradient table of provided
bval and bvec. (default: 0.0129)
big_delta :

This comment has been minimized.

@skoudoro

skoudoro Oct 31, 2017

Member

add the datatype -> big_delta : float

big_delta :
Big delta value used in generation of gradient table of provided
bval and bvec. (default: 0.0218)
save_metrics :

This comment has been minimized.

@skoudoro

skoudoro Oct 31, 2017

Member

add the datatype -> save_metric : string

big_delta=big_delta, b0_threshold=50)
data_small = data[60:85, 80:81, 60:85]

This comment has been minimized.

@skoudoro

skoudoro Oct 31, 2017

Member

Why do you crop the data? if it is really necessary, you have to warn the user that only a small part of its data will be processed. Otherwise, use the whole data.

@arokem

This comment has been minimized.

Member

arokem commented Oct 31, 2017

@aarya22

This comment has been minimized.

Contributor

aarya22 commented Nov 2, 2017

The reason I cropped the data and had the output be a png file is because I wanted this to be a simple preliminary test for making a brain-life application from a dipy workflow. I was thinking that eventually brain-life applications can simply become wrappers for the dipy workflow.

I agree that png files should not be an output of the workflow, but I am not sure what else I should return from Mapmir. I do not know enough about the model, if you have any suggestions I will gladly take them.

@arokem

This comment has been minimized.

Member

arokem commented Nov 2, 2017

@aarya22 aarya22 force-pushed the aarya22:master branch from 6784b17 to 61f7729 Nov 22, 2017

@aarya22 aarya22 changed the title from Add mapmri flow to WIP: Add mapmri flow Nov 26, 2017

aarya22 added some commits Nov 27, 2017

aarya22 added some commits Dec 7, 2017

@arokem arokem referenced this pull request Dec 7, 2017

Closed

Mapmri workflow #1388

@aarya22

This comment has been minimized.

Contributor

aarya22 commented Dec 8, 2017

@skoudoro @arokem I've managed to get the Travis build test work for most of the environments, but there are a few environments that do not provide the CVXPY package which is necessary for the workflow to finish. I'm not sure how to access the config so I can include the CVXPY package in all the environments.

@arokem

This comment has been minimized.

Member

arokem commented Dec 8, 2017

aarya22 added some commits Dec 10, 2017

Merge pull request #4 from aarya22/MAPMRI_Workflow
Add decorators to remove cvxpy from testing
@aarya22

This comment has been minimized.

Contributor

aarya22 commented Dec 10, 2017

@skoudoro @arokem Travis has passed, but I'm not sure what the other two errors are about or what I am supposed to be changing there.

@arokem arokem referenced this pull request Jan 1, 2018

Open

DataType and input files #3

@arokem

A few smaller comments, and questions about the overall design. @Garyfallidis : could you please take a look at this. Are we really supposed to have a separate object for every option of the model? That is a separate object for Laplacian regularization, for the positivity constraint, and for both?

return 'mapmri'
def run(self, data_file, data_bvecs, data_bvals, model_type='', out_rtop='rtop.nii.gz',
out_lapnorm='lapnorm.nii.gz',out_msd='msd.nii.gz', out_qiv='qiv.nii.gz', out_rtap='rtap.nii.gz',

This comment has been minimized.

@arokem

arokem Jan 1, 2018

Member

PEP8: this line is >80 characters

out_lapnorm='lapnorm.nii.gz',out_msd='msd.nii.gz', out_qiv='qiv.nii.gz', out_rtap='rtap.nii.gz',
out_rtpp='rtpp.nii.gz', small_delta=0.0129, big_delta=0.0218, save_metrics=[], out_dir=''):
""" Workflow for the app-dipy-mapmri on Brain-Life (www.brain-life.org).
Generates rtop, lapnorm, msd, qiv, rtap, rtpp saved in a nifti format in input files provided by

This comment has been minimized.

@arokem

arokem Jan 1, 2018

Member

PEP8: line is >80 characters

io_it = self.get_io_iterator()
for dwi, bval, bvec, out_rtop, out_lapnorm, out_msd, out_qiv, out_rtap, out_rtpp in io_it:
logging.info('Computing DTI metrics for {0}'.format(dwi))

This comment has been minimized.

@arokem

arokem Jan 1, 2018

Member

This should be "Computing MAPMRI metrics..."

def get_short_name(cls):
return 'mapmri'
def run(self, data_file, data_bvecs, data_bvals, model_type='', out_rtop='rtop.nii.gz',

This comment has been minimized.

@arokem

arokem Jan 1, 2018

Member

Instead of model_type, I would suggest having a keyword argument for "laplacian" and for "positivity" (both set to True per default)

This comment has been minimized.

@arokem

arokem Jan 1, 2018

Member

And a keyword argument for the laplacian weighting as well.

positivity_constraint=True)
mapfit_aniso = map_model_aniso.fit(data)
# Not sure where to get affine or metadata?

This comment has been minimized.

@arokem

arokem Jan 1, 2018

Member

Is this still an issue? Getting the affine? You can get it from the data_file.

As an aside: if you have questions, it's better to ask them in the PR interface than put them as comments in your code!

This comment has been minimized.

@aarya22

aarya22 Jan 2, 2018

Contributor

Ah thanks for pointing that out. I added those notes in the code to ask you at our weekly sessions and forgot to delete them.

format(os.path.dirname(out_rtpp)))
class ReconstMAPMRILaplacian(ReconstMAPMRIFlow):

This comment has been minimized.

@arokem

arokem Jan 1, 2018

Member

@Garyfallidis : is this the right way to do things? You create an interface for every setting of options to the model? I thought this was supposed to create a CLI that allows you to input key-word arguments?

@aarya22 aarya22 referenced this pull request Jan 3, 2018

Closed

Mapmri workflow #1395

@arokem

This comment has been minimized.

Member

arokem commented Jan 3, 2018

Closed in favor of #1395

@arokem arokem closed this Jan 3, 2018

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