ENH: Use the CFIN dataset rather than the CENIR dataset. #1424

jhlegarreta · 2018-02-11T14:37:20Z

Following the discussion in #1291, use the CFIN dataset instead of
the CENIR dataset.

Also, make an explicit mention to the MAPL acronym so that the use of
the Laplacian-regularized MAP-MRI method seems no longer to be hidden.

jhlegarreta · 2018-02-11T14:41:46Z

@Garyfallidis @arokem

I removed the

For this example we select only the shell with b-values equal to the one of the Human Connectome Project (HCP).

sentence from the documentation since read_cfin_dwi does not accept the b-value list as an argument.

Finally, I got an error while debugging the example:

  File "C:/SDKs/dipy/doc/examples/reconst_mapmri.py", line 124, in <module>
    positivity_constraint=True)
  File "C:\SDKs\dipy\dipy\reconst\mapmri.py", line 251, in __init__
    'CVXPY package needed to enforce constraints')
ValueError: CVXPY package needed to enforce constraints

at

map_model_positivity_aniso = mapmri.MapmriModel(gtab,
                                                radial_order=radial_order,
                                                laplacian_regularization=False,
                                                positivity_constraint=True)

If CI builds do not report issues, I can assume this is a local issue, and that our requirements.txt does not need to be updated to include the CVXPY package. Correct me if I'm wrong.

codecov-io · 2018-02-11T16:58:35Z

Codecov Report

Merging #1424 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1424      +/-   ##
==========================================
- Coverage   87.51%   87.49%   -0.02%     
==========================================
  Files         241      241              
  Lines       30724    30724              
  Branches     3326     3326              
==========================================
- Hits        26887    26883       -4     
- Misses       3060     3063       +3     
- Partials      777      778       +1

Impacted Files	Coverage Δ
dipy/reconst/forecast.py	`90.15% <0%> (-2.08%)`	⬇️

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 b1c8079...3eb7ddb. Read the comment docs.

jhlegarreta · 2018-02-12T16:22:57Z

Looks like CircleCI containers do have the CVXPY package installed. Any comments on requiring it?

I've noticed that the CENIR data has also data for b-vals 200 and 400 s/mm2. The plots in the example will not show data for all data since they only contain three subplots; should we show all data, or just take into account only the 1000, 2000 and 3000 s/mm2, as the previous version was doing?

skoudoro · 2018-03-06T23:44:19Z

Can you review this PR @arokem?

arokem · 2018-03-07T00:47:23Z

Will do!

arokem · 2018-03-08T05:31:26Z

Moving to this new dataset will require some additional changes. See jhlegarreta#1

jhlegarreta · 2018-03-10T18:27:37Z

Thanks for the review @arokem !

arokem · 2018-03-10T21:15:13Z

Thank you! I am +1 for merging this. If someone else could take a look, that would be great.

RicciWoo · 2018-03-26T11:07:33Z

After PR: mapmri using cvxpy instead of cvxopt #1285, we are now using package CVXPY instead of CVXOPT, I think we need to update the comment on line 27-31 in reconst_mapmripy for this.

jhlegarreta · 2018-03-26T20:18:58Z

Thanks for the comments.

@RicciWoo @arokem Please see whether commit 801a42f addresses adequately this.

I did not add the reference to the cvxpy package in doc/links_names.inc, so if this gets merged before #1474, we will need to rebase the latter and use the link in reconst_mapmri.py.

I haven't mentioned the conda part, since this a more general comment, applicable to any package.

skoudoro · 2018-04-04T13:40:26Z

Hi @jhlegarreta, I got this error when I tried to run this example with the new dataset:

Traceback (most recent call last):
  File "reconst_mapmri.py", line 162, in <module>
    vmin=0, vmax=5e7)
  File "C:\Users\skoudoro\AppData\Local\Continuum\Anaconda3\envs\dipy-dev-3\lib\site-packages\matplotlib\__init__.py", line 1897, in inner
    return func(ax, *args, **kwargs)
  File "C:\Users\skoudoro\AppData\Local\Continuum\Anaconda3\envs\dipy-dev-3\lib\site-packages\matplotlib\axes\_axes.py", line 5124, in imshow
    im.set_data(X)
  File "C:\Users\skoudoro\AppData\Local\Continuum\Anaconda3\envs\dipy-dev-3\lib\site-packages\matplotlib\image.py", line 596, in set_data
    raise TypeError("Image data can not convert to float")
TypeError: Image data can not convert to float

I am working on Windows. Do you get this error ? Can you update the plotting part.
Thank you

jhlegarreta · 2018-04-07T17:04:55Z

@skoudoro thanks for the review. I did get the same error.

Please, check if commit 795aa4f solves the plot part issues.

Also, I am getting an error at

/dipy/doc/examples/reconst_mapmri.py", line 393, in <module>
    print('odf.shape (%d, %d, %d, %d)' % odf.shape)

telling me that

TypeError: not enough arguments for format string

Let's see what CI says, or whether you do get the same error; if you do, I'll amend with

print('odf.shape ({0})'.format(odf.shape))

BTW, although I know other parts of the code use cvxpy, in order to run the exampled I had to manually install it (pip was not listing it either as installed), so I'm wondering whether cvxpy should be added to the requirements.txt file.

May be this is a Windows-specific issue (?)

jhlegarreta · 2018-04-07T18:30:20Z

Looks like travis is happy; may be because no image is run on Windows machines (?). @skoudoro let me know when you check the patch set.

arokem · 2018-04-07T18:37:35Z

It's because the examples are not run as part of the CI. That would take way too long.

…

On Sat, Apr 7, 2018 at 6:30 PM, Jon Haitz Legarreta Gorroño < ***@***.***> wrote: Looks like travis is happy; may be because no image is run on Windows machines (?). @skoudoro <https://github.com/skoudoro> let me know when you check the patch set. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1424 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNtnZ0IyEAsLuCWIxDubeTPD262Kkks5tmQW9gaJpZM4SBXE6> .

skoudoro · 2018-04-07T18:57:36Z

Thanks @jhlegarreta. Today, I do not have access to my windows machine so I will try it tomorrow.

Concerning cvxpy, I think we want to let it as an optional requirement so do not add it to requirement.txt

skoudoro

Hi @jhlegarreta, the new dataset has this shape (96,96,19,496) so can you update the crop to make it work? look at the line here and replace it with something like data[40:65, 50:51]

Can you rebase this PR?

thanks!

Following the discussion in dipy#1291, use the ``CFIN`` dataset instead of the ``CENIR`` dataset. Also, make an explicit mention to the `MAPL` acronym so that the use of the Laplacian-regularized MAP-MRI method seems no longer to be hidden.

jhlegarreta · 2018-04-22T20:14:44Z

@skoudoro 3eb7ddb was rebased on master.

As for the data region, just to make sure, you mean using a slice instead of a volume for the example now?

Regardless of the data region, not sure whether the result images are correct: I get all 3 images in MAPMRI_maps_regularization.png black.

skoudoro · 2018-04-22T20:34:49Z

As for the data region, just to make sure, you mean using a slice instead of a volume for the example now?

print(data.shape) -> (96,96,19,496)
data_small = data[40:65, 50:51, 35:60] -> as you can see here 35:60do not exist on the new dataset. In both case (old, and new dataset), it seems we work with one slice.

I get all 3 images in MAPMRI_maps_regularization.png black

In interactive mode, all results seem good when we display them, so maybe a saving issue, I will look at that

jhlegarreta · 2018-04-22T20:56:48Z

OK. Let me know if you are able to reproduce the issue (working on Win 10); if not, may be it's local to my environment and we can ignore it. Thanks Serge.

skoudoro

Hi @jhlegarreta, below, the answer of black images.

Can you add sfu.RotateX(-90) line 390

It will be ready to merge after all this change.

Thank you @jhlegarreta

skoudoro · 2018-04-28T17:59:58Z

doc/examples/reconst_mapmri.py

+rtop_laplacian = np.array(mapfit_laplacian_aniso.rtop()[:, 0, :].T,
+                          dtype=float)
+ind = ax1.imshow(rtop_laplacian, interpolation='nearest',
+                 origin='lower', cmap=plt.cm.gray, vmin=0, vmax=5e7)


vmin and vmax are incorrect. This wrong range is the reason for your black image. Can you just remove this 2 parameters and let matplotlib manage it

skoudoro · 2018-04-28T18:00:09Z

doc/examples/reconst_mapmri.py

+rtop_positivity = np.array(mapfit_positivity_aniso.rtop()[:, 0, :].T,
+                           dtype=float)
+ind = ax2.imshow(rtop_positivity, interpolation='nearest',
+                 origin='lower', cmap=plt.cm.gray, vmin=0, vmax=5e7)


vmin and vmax are incorrect. This wrong range is the reason for your black image. Can you just remove this 2 parameters and let matplotlib manage it

skoudoro · 2018-04-28T18:00:23Z

doc/examples/reconst_mapmri.py

-                 vmin=0, vmax=5e7)
+rtop_both = np.array(mapfit_both_aniso.rtop()[:, 0, :].T, dtype=float)
+ind = ax3.imshow(rtop_both, interpolation='nearest', origin='lower',
+                 cmap=plt.cm.gray, vmin=0, vmax=5e7)


vmin and vmax are incorrect. This wrong range is the reason for your black image. Can you just remove this 2 parameters and let matplotlib manage it

skoudoro · 2018-04-30T15:43:13Z

Due to the release, I will merge it and make the last correction on new PR.

Thank you @jhlegarreta for this PR and @arokem for this review!

jhlegarreta · 2018-05-02T17:18:24Z

@skoudoro @arokem thanks for the follow-up and for having fixed the remaining issues.

…taInReconstMAPMRIExample ENH: Use the CFIN dataset rather than the CENIR dataset.

jhlegarreta force-pushed the UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from e6c97b1 to 9190ba6 Compare February 11, 2018 14:40

arokem mentioned this pull request Mar 26, 2018

DOC: Add comment about package CVXPY in example of reconst_mapmri.py #1478

Closed

jhlegarreta force-pushed the UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from d768bc4 to 801a42f Compare March 26, 2018 20:18

jhlegarreta force-pushed the UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from 801a42f to 795aa4f Compare April 7, 2018 17:01

skoudoro reviewed Apr 19, 2018

View reviewed changes

jhlegarreta force-pushed the UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from 795aa4f to 3eb7ddb Compare April 22, 2018 20:12

skoudoro reviewed Apr 28, 2018

View reviewed changes

skoudoro merged commit ae88db5 into dipy:master Apr 30, 2018

This was referenced Apr 30, 2018

fix plotting issue on Mapmri example #1503

Merged

Existing MAPMRI tutorial does not render correctly and MAPL looks hidden in the existing tutorial. #1291

Closed

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request dipy#1424 from jhlegarreta/UseCFINMultibValueDWIDa…

48d5d1f

…taInReconstMAPMRIExample ENH: Use the CFIN dataset rather than the CENIR dataset.

jhlegarreta deleted the UseCFINMultibValueDWIDataInReconstMAPMRIExample branch October 10, 2018 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Use the CFIN dataset rather than the CENIR dataset. #1424

ENH: Use the CFIN dataset rather than the CENIR dataset. #1424

jhlegarreta commented Feb 11, 2018

jhlegarreta commented Feb 11, 2018

codecov-io commented Feb 11, 2018 •

edited

jhlegarreta commented Feb 12, 2018

skoudoro commented Mar 6, 2018

arokem commented Mar 7, 2018

arokem commented Mar 8, 2018

jhlegarreta commented Mar 10, 2018

arokem commented Mar 10, 2018

RicciWoo commented Mar 26, 2018

jhlegarreta commented Mar 26, 2018

skoudoro commented Apr 4, 2018

jhlegarreta commented Apr 7, 2018

jhlegarreta commented Apr 7, 2018

arokem commented Apr 7, 2018 via email

skoudoro commented Apr 7, 2018

skoudoro left a comment

jhlegarreta commented Apr 22, 2018

skoudoro commented Apr 22, 2018

jhlegarreta commented Apr 22, 2018

skoudoro left a comment

skoudoro Apr 28, 2018

skoudoro Apr 28, 2018

skoudoro Apr 28, 2018

skoudoro commented Apr 30, 2018

jhlegarreta commented May 2, 2018

ENH: Use the CFIN dataset rather than the CENIR dataset. #1424

ENH: Use the CFIN dataset rather than the CENIR dataset. #1424

Conversation

jhlegarreta commented Feb 11, 2018

jhlegarreta commented Feb 11, 2018

codecov-io commented Feb 11, 2018 • edited

Codecov Report

jhlegarreta commented Feb 12, 2018

skoudoro commented Mar 6, 2018

arokem commented Mar 7, 2018

arokem commented Mar 8, 2018

jhlegarreta commented Mar 10, 2018

arokem commented Mar 10, 2018

RicciWoo commented Mar 26, 2018

jhlegarreta commented Mar 26, 2018

skoudoro commented Apr 4, 2018

jhlegarreta commented Apr 7, 2018

jhlegarreta commented Apr 7, 2018

arokem commented Apr 7, 2018 via email

skoudoro commented Apr 7, 2018

skoudoro left a comment

Choose a reason for hiding this comment

jhlegarreta commented Apr 22, 2018

skoudoro commented Apr 22, 2018

jhlegarreta commented Apr 22, 2018

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro Apr 28, 2018

Choose a reason for hiding this comment

skoudoro Apr 28, 2018

Choose a reason for hiding this comment

skoudoro Apr 28, 2018

Choose a reason for hiding this comment

skoudoro commented Apr 30, 2018

jhlegarreta commented May 2, 2018

codecov-io commented Feb 11, 2018 •

edited