Skip to content
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

NF: MSMT - CSD #1858

Merged
merged 33 commits into from Jun 25, 2019

Conversation

@ShreyasFadnavis
Copy link
Member

commented Jun 14, 2019

  • Replace QP from CVXOPT with CVXPY
  • Fix Example
  • Add tests
  • Add docstrings
@pep8speaks

This comment has been minimized.

Copy link

commented Jun 14, 2019

Hello @ShreyasFadnavis, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2019-06-24 17:51:48 UTC
del

@ShreyasFadnavis ShreyasFadnavis changed the title [WIP] NF: MSMT - CSD NF: MSMT - CSD Jun 17, 2019

@arokem
Copy link
Member

left a comment

Strong work! I had some questions/comments. I realize that maybe some of these are for @MrBago.

dipy/reconst/mcsd.py Show resolved Hide resolved
return self.response.shape[1] - (self.sh_order // 2) - 1


def closest(haystack, needle):

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

This function is:

  1. two lines implementing a fairly straightforward use of numpy,
  2. not documented or directly tested, and
  3. used exactly once.

Could you just move this code into the calling function and eliminate this?

"""Simple delta function
Parameters
----------
iso: int (optional)

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

Says it's optional, but it doesn't seem to be optional in the function signature.

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

That said, it would make sense to add error handling to make sure that it's at least 2.

Parameters
----------
iso: int (optional)

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

Same comment here.


B = shm.real_sph_harm(m, n, t[:, None], p[:, None])
G_temp = np.ascontiguousarray(B[:, n != 0])
# c_ samples the delta function at the delta orientation.

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

This comment seems to refer to a variable that no longer exists (?).

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

Or maybe this is c_temp?


def quadprog(P, Q, G, H):
r"""
Helper funstion to set up the Quadratic Program (QP) solver in CVXPY.

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

Typo: funstion => function


def quadprog(P, Q, G, H):
r"""
Helper funstion to set up the Quadratic Program (QP) solver in CVXPY.

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

This documentation is a bit misleading. This doesn't just set up the problem. It sets it up and then solves it.

minimize 1/2 x' P x + Q' x
subject to G x <= H
Here the QP solver is based on CVXPY and uses OSQP by default.

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

Both here and above, the solver is not a default that can be changed. This should be something like "Here the QP solver is based on CVXPY and uses OSQP." without that last phrase.

npt.assert_array_almost_equal(fit.shm_coeff[m != 0], 0., 2)


test_mcsd_model_delta()

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

Why are you calling this here?


sh_order = 8
response = sim_response(sh_order, [0, 1000, 2000, 3500])
model = MultiShellDeconvModel(gtab, response,

This comment has been minimized.

Copy link
@arokem

arokem Jun 18, 2019

Member

Do you want to also test the version without positivity constraint?

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jun 18, 2019

Author Member

Done!

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

Strong work! I had some questions/comments. I realize that maybe some of these are for @MrBago.

Thank you so much @arokem :) Will address all the changes mentioned above ASAP!

@codecov-io

This comment has been minimized.

Copy link

commented Jun 18, 2019

Codecov Report

Merging #1858 into master will increase coverage by 6.99%.
The diff coverage is 95.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1858      +/-   ##
==========================================
+ Coverage   83.59%   90.58%   +6.99%     
==========================================
  Files         117      242     +125     
  Lines       14234    29950   +15716     
  Branches     2251     3169     +918     
==========================================
+ Hits        11899    27131   +15232     
- Misses       1813     2163     +350     
- Partials      522      656     +134
Impacted Files Coverage Δ
dipy/reconst/shm.py 85.98% <100%> (ø) ⬆️
dipy/reconst/mcsd.py 93.85% <93.85%> (ø)
dipy/reconst/tests/test_mcsd.py 97.64% <97.64%> (ø)
dipy/workflows/io.py 74.46% <0%> (-15.33%) ⬇️
dipy/reconst/tests/test_peakdf.py 95.74% <0%> (ø)
dipy/align/tests/test_expectmax.py 88.75% <0%> (ø)
dipy/reconst/tests/test_ivim.py 93.71% <0%> (ø)
dipy/workflows/tests/test_reconst_csa_csd.py 95.5% <0%> (ø)
dipy/io/tests/__init__.py 100% <0%> (ø)
dipy/reconst/tests/test_gqi.py 90.9% <0%> (ø)
... and 129 more

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 703cd83...5ea2ed4. Read the comment docs.

@MrBago
Copy link
Contributor

left a comment

Let me know if you have more questions for me :)

B = shm.real_sph_harm(m, n, t[:, None], p[:, None])
G_temp = np.ascontiguousarray(B[:, n != 0])
# c_ samples the delta function at the delta orientation.
c_temp = G_temp[0][:, None]

This comment has been minimized.

Copy link
@MrBago

MrBago Jun 18, 2019

Contributor

c_temp should be shm.real_sph_harm(m[n != 0], n[n != 0], theta(x, y, z), phi(x, y, z). Since new_vertices are reoriented so that new_vertices[0] == [x, y, z], we can sample the matrix generated from new_vertices.

# n == 0 is set to sh_const to ensure a normalized delta function.
# n > 0 values are optimized so that delta > 0 on all points of the sphere
# and delta(theta, phi) is maximized.
lp_prob = cvx.Problem(cvx.Maximize(cvx.sum(c_temp)), [G*x <= h])

This comment has been minimized.

Copy link
@MrBago

MrBago Jun 18, 2019

Contributor

This doesn't seem right, is this x the same as the x defined in on 132? I thing x should be a variable & should be referenced in the Maximize.

I'm not sure how useful the _pos_constrained_delta is, consider removing it for this PR and adding it later with better tests.

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jun 18, 2019

Author Member

Will do.. I was about to ask the same..! Will take this function down for now :)

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@arokem @MrBago Travis seems happy now! Any other comments, refactoring or suggestions? Also, I'm testing this with some HCP dataset.. Any other data that you have in mind for the tutorial? Stanford HARDI is an option I guess!

Will follow up with another PR for the tutorial and workflow once this is merged :)

@MrBago

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@arokem

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

@arokem
Copy link
Member

left a comment

A couple more comments here. Getting closer! Are you planning to add an example on this PR?

dipy/reconst/mcsd.py Show resolved Hide resolved
dipy/reconst/mcsd.py Show resolved Hide resolved
ndarray and the second is the signal value for the response
function without diffusion weighting. This is to be able to
generate a single fiber synthetic signal.
sh_order : ndarray

This comment has been minimized.

Copy link
@arokem

arokem Jun 20, 2019

Member

Is this really an array? Why?

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jun 20, 2019

Author Member

Is this really an array? Why?

My bad!

dipy/reconst/mcsd.py Show resolved Hide resolved
def __init__(self, model, coeff, mask):
"""
Inherits the SphHarmFit which fits the diffusion data to a spherical
harmic model.

This comment has been minimized.

Copy link
@arokem

arokem Jun 20, 2019

Member

Typo: "harmic" => "harmonic"

dipy/reconst/mcsd.py Show resolved Hide resolved
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Oh - just saw your comment - tutorial on separate PR sounds good. So, there's just a couple of small things to resolve here and it will be ready to merge.

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

A couple more comments here. Getting closer! Are you planning to add an example on this PR?

Hi @arokem ! Thank you for the suggestions! I will incorporate all the changes 👍
I will follow up with another PR for the example and workflow :)

The example has multiple steps and I feel that it will make this PR too big.

@ShreyasFadnavis ShreyasFadnavis requested a review from MrBago Jun 20, 2019

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2019

@arokem is this good now? I would like to create one more PR for the tutorial for this model for the 1.0 release... Let me know in case of any more changes :)

@arokem
Copy link
Member

left a comment

Almost there. Just need to document these.


def multi_tissue_basis(gtab, sh_order, iso_comp):
"""
Builds a basis for multi-shell CSD model.

This comment has been minimized.

Copy link
@arokem

arokem Jun 24, 2019

Member

Documentation for this function is still not complete.

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jun 24, 2019

Author Member

Almost there. Just need to document these.

Sorry for that.. On it!

dipy/reconst/mcsd.py Show resolved Hide resolved
dipy/reconst/mcsd.py Outdated Show resolved Hide resolved
Update dipy/reconst/mcsd.py
Co-Authored-By: Ariel Rokem <arokem@gmail.com>
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Thanks @ShreyasFadnavis. Merging!

Please do follow up with an example.

@arokem arokem merged commit deedcdf into nipy:master Jun 25, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 95.5% of diff hit (target 83.59%)
Details
codecov/project 90.58% (+6.99%) compared to 703cd83
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.