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

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

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
5 participants
@jhlegarreta
Contributor

jhlegarreta commented Feb 11, 2018

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 jhlegarreta force-pushed the jhlegarreta:UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from e6c97b1 to 9190ba6 Feb 11, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Feb 11, 2018

@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

This comment has been minimized.

codecov-io commented Feb 11, 2018

Codecov Report

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

Impacted file tree graph

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

This comment has been minimized.

Contributor

jhlegarreta commented Feb 12, 2018

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

This comment has been minimized.

Member

skoudoro commented Mar 6, 2018

Can you review this PR @arokem?

@arokem

This comment has been minimized.

Member

arokem commented Mar 7, 2018

Will do!

@arokem

This comment has been minimized.

Member

arokem commented Mar 8, 2018

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

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Mar 10, 2018

Thanks for the review @arokem !

@arokem

This comment has been minimized.

Member

arokem commented Mar 10, 2018

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

@RicciWoo

This comment has been minimized.

Contributor

RicciWoo commented Mar 26, 2018

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 jhlegarreta force-pushed the jhlegarreta:UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from d768bc4 to 801a42f Mar 26, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Mar 26, 2018

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

This comment has been minimized.

Member

skoudoro commented Apr 4, 2018

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 jhlegarreta force-pushed the jhlegarreta:UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from 801a42f to 795aa4f Apr 7, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Apr 7, 2018

@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

This comment has been minimized.

Contributor

jhlegarreta commented Apr 7, 2018

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

This comment has been minimized.

Member

arokem commented Apr 7, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Apr 7, 2018

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!

Jon Haitz Legarreta
ENH: Use the CFIN dataset rather than the CENIR dataset.
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 jhlegarreta force-pushed the jhlegarreta:UseCFINMultibValueDWIDataInReconstMAPMRIExample branch from 795aa4f to 3eb7ddb Apr 22, 2018

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Apr 22, 2018

@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

This comment has been minimized.

Member

skoudoro commented Apr 22, 2018

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

This comment has been minimized.

Contributor

jhlegarreta commented Apr 22, 2018

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

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)

This comment has been minimized.

@skoudoro

skoudoro Apr 28, 2018

Member

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

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)

This comment has been minimized.

@skoudoro

skoudoro Apr 28, 2018

Member

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

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)

This comment has been minimized.

@skoudoro

skoudoro Apr 28, 2018

Member

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

This comment has been minimized.

Member

skoudoro commented Apr 30, 2018

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!

@skoudoro skoudoro merged commit ae88db5 into nipy:master Apr 30, 2018

2 of 3 checks passed

codecov/project 87.49% (-0.02%) compared to b1c8079
Details
codecov/patch Coverage not affected when comparing b1c8079...3eb7ddb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented May 2, 2018

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

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

Merge pull request nipy#1424 from jhlegarreta/UseCFINMultibValueDWIDa…
…taInReconstMAPMRIExample

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

@jhlegarreta jhlegarreta deleted the jhlegarreta:UseCFINMultibValueDWIDataInReconstMAPMRIExample branch Oct 10, 2018

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