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

NODDIx 2 fibers crossing #1600

Closed
wants to merge 1,650 commits into
base: master
from

Conversation

Projects
None yet
@ShreyasFadnavis
Contributor

ShreyasFadnavis commented Jul 26, 2018

  • Create Example for the NODDIx model

skoudoro and others added some commits Apr 13, 2018

Merge pull request #1497 from RicciWoo/bf-ex-fiber_to_bundle_coherence
BF: fix bug in example of fiber_to_bundle_coherence.py
Merge pull request #1493 from dmreagan/iss1490
Fix surface rendering bugs in VTK8
Jon Haitz Legarreta
DOC: Fix `reconst_mapmri` example markup.
Fix formatting issues in the `reconst_mapmri` file to get the
documentation correctly rendered in:
http://nipy.org/dipy/examples_built/reconst_mapmri.html#example-reconst-mapmri

Changes:
- Indent lists properly.
- Indent literal blocks properly.
- Remove unnecessary double quotes for indented code/literal blocks.
- Use double backquotes for inline code samples.
Merge pull request #1423 from jhlegarreta/FixReconstMAPMRIExampleForm…
…atting

DOC: Fix `reconst_mapmri` example markup.
Merge pull request #1447 from jhlegarreta/LinkCodingStyleGuideRefToWe…
…bsiteDocs

DOC: Link Coding Style Guide ref in CONTRIBUTING to the website.
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.
Merge pull request #1443 from Garyfallidis/rb_rb
RecoBundles - recognition of bundles
@ShreyasFadnavis

This comment has been minimized.

Contributor

ShreyasFadnavis commented Aug 12, 2018

Complete Description of PR in the PDF below:

GSoC 2018 - PSF - DIPY report.pdf

@skoudoro skoudoro referenced this pull request Sep 12, 2018

Closed

[WIP] Needs Optimization and Cleaning #1576

0 of 3 tasks complete
@skoudoro

Hi @ShreyasFadnavis, Thank you for the update. Can you:

  • Rebase your code
  • Fix all pep8 issue?
  • Fix the documentation of all your functions?
  • Add 2 tutorials (for sims, and for Noddix)
  • Add more testing

Thank you

@@ -30,6 +30,7 @@ def new_fit(self, data, mask=None):
fit_array = np.empty(data.shape[:-1], dtype=object)
for ijk in ndindex(data.shape[:-1]):
if mask[ijk]:
print(ijk)

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

can you remove this print ?

This comment has been minimized.

@ShreyasFadnavis
@@ -288,6 +288,10 @@ def get_data(name='small_64D'):
return pjoin(DATA_DIR, 'cb_2.npz')
if name == "t1_coronal_slice":
return pjoin(DATA_DIR, 't1_coronal_slice.npy')
if name == "small_NODDIx_data":
fimg = pjoin(DATA_DIR, 'small_NODDIx_HCP.hdr')

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

This file is missing.

This comment has been minimized.

@ShreyasFadnavis
"""
def __init__(self, gtab, params, fit_method='MIX'):
# The maximum number of generations, genetic algorithm 1000 default, 1

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

Can you add a doc on this __init__ function?

This comment has been minimized.

@ShreyasFadnavis
self.xtol = 1e-8
self.gtab = gtab
self.big_delta = gtab.big_delta
self.small_delta = gtab.small_delta

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

big_delta and small_delta are already saved in self.gtab so no need to create this variable (self.big_delta and self.small_delta)

self.gtab = gtab
self.big_delta = gtab.big_delta
self.small_delta = gtab.small_delta
self.gamma = gamma

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

variable not used. I think you should self.gamma in your class

L[x, n] = Integrate[Exp[-x \mu^2] Legendre[2*n, \mu], {\mu, -1, 1}]
L[x, n] = Integrate[Exp[-x \mu^2] (-\mu^2) Legendre[2*n, \mu], {\mu, -1, 1}]
INPUTS:

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

Parameters instead

order of legendre polynomial
The maximum value for n is 6.
OUTPUTS:

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

Returns instead

order and the derivatives if requested.
Truncating at the 12th order gives good approximation for kappa up to 64.
INPUTS:

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

same as above

k should be an array of positive numbers, specifying a set of
concentration parameters for the Watson's distribution.
OUTPUTS:

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

same as above

f = x_f_sig[0:5]
phi = noddix_model.Phi2(x_f_sig)
reconst_signal = np.dot(phi, f)

This comment has been minimized.

@skoudoro

skoudoro Sep 12, 2018

Member

Can you create a setup function for the bloc above?

@Garyfallidis Garyfallidis changed the title from PR for NODDIx 2 Crossing to NODDIx 2 fibers Crossing Sep 20, 2018

@Garyfallidis Garyfallidis changed the title from NODDIx 2 fibers Crossing to NODDIx 2 fibers crossing Sep 20, 2018

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