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

RecoBundles and SLR workflows #1556

Merged
merged 106 commits into from Oct 11, 2018

Conversation

Projects
None yet
8 participants
@BramshQamar
Copy link
Contributor

BramshQamar commented Jun 8, 2018

This pull request introduces:

  • SLR workflow

  • RecoBundles workflow

  • Apply Labels workflow

  • Refinement method for RecoBundles

This PR still needs:

  • Fetcher for model bundles

  • Fetcher for whole brain subject

  • Tutorial

  • More tests needed

  • Use RandomState

Garyfallidis and others added some commits May 8, 2018

Merge pull request #54 from BramshQamar/rb_workflows
NF: adding RecoBundles workflow
Merge pull request #55 from BramshQamar/rb_workflows
Slr and apply_labels workflows

BramshQamar added some commits Sep 7, 2018

@Garyfallidis Garyfallidis changed the title WIP: RecoBundles and SLR workflows RecoBundles and SLR workflows Sep 21, 2018

BramshQamar added some commits Sep 24, 2018

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Sep 27, 2018

AFAIK This is ready to be merged. Please comment as needed. If nobody comments will merge this PR in 48 hours.

@arokem
Copy link
Member

arokem left a comment

Nice! Not really a thorough review, but I found a few places where cruft could be removed.

Other than that, +1 for the merge!

@@ -1,5 +1,5 @@
from __future__ import division, print_function, absolute_import

#fetcher

This comment has been minimized.

@arokem

arokem Sep 27, 2018

Member

This doesn't need to be here, right?

d01 = bundles_distances_mdf(dtracks0, dtracks1)

pair12 = []
# solo1 = []

This comment has been minimized.

@arokem

arokem Sep 27, 2018

Member

Remove this.

if np.min(d01[i, :]) < threshold:
j = np.argmin(d01[i, :])
pair12.append((i, j))
# else:

This comment has been minimized.

@arokem

arokem Sep 27, 2018

Member

Remove this

if np.min(d01[:, i]) < threshold:
j = np.argmin(d01[:, i])
pair21.append((i, j))
# else:

This comment has been minimized.

@arokem

arokem Sep 27, 2018

Member

Remove this

@@ -0,0 +1,172 @@
import numpy as np

This comment has been minimized.

@arokem

arokem Sep 27, 2018

Member

Should this be done before merging?

@skoudoro
Copy link
Member

skoudoro left a comment

Here, the first part of my review. There are some pep8 issues that I did not comment, Can you check them ?

from dipy.segment.clustering import QuickBundles
length,
Streamlines)
from dipy.segment.clustering import QuickBundles, qbx_and_merge

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

QuickBundles is unused, can you remove it from this import?

from dipy.core.geometry import (compose_transformations,
compose_matrix,
decompose_matrix)
from dipy.utils.six import string_types
from time import time
from dipy.tracking.streamline import Streamlines

MAX_DIST = 1e10

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

Why MAX_DISTis global, you use it only once

from dipy.core.geometry import (compose_transformations,
compose_matrix,
decompose_matrix)
from dipy.utils.six import string_types
from time import time
from dipy.tracking.streamline import Streamlines

MAX_DIST = 1e10
LOG_MAX_DIST = np.log(MAX_DIST)

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

LOG_MAX_DIST is not used. Can you remove it?

@@ -693,8 +695,15 @@ def bundle_min_distance_asymmetric_fast(t, static, moving, block_size):


def remove_clusters_by_size(clusters, min_size=0):

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

Can you add documentation for this function?

less_than=250,
qbx_thr=[40, 30, 20, 15],
nb_pts=20,
progressive=True, rng=None, num_threads=None):
""" Utility function for registering large tractograms.

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

Can you add maxiter, progressive, nb_pts, qbx_thr, less_than, greater_than in the docstring ?

clusters2 = remove_clusters_by_size(cluster_map2, rm_small_clusters)
qb_centroids2 = [cluster.centroid for cluster in clusters2]
qb_centroids2 = clusters2

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

Why you do not do directly qb_centroids2 = remove_clusters_by_size(cluster_map2, rm_small_clusters)?

clusters2 variable seems useless

qb_thr=15,
nb_pts=20,
progressive=True, num_threads=None):
def slr_with_qbx(static, moving,

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

pep8: expected 2 blanks lines

'bundles',
'*.trk')

return file1, file2

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

I expect the read function to return a Streamlines object and not filename. Indeed, the fetcher already returns file names... I think you should redesign this function.

This comment has been minimized.

@BramshQamar

BramshQamar Oct 8, 2018

Contributor

Hi Serge,

Thanks for the review. We decided to return just file location from read function because otherwise we would have to load all data files and return them, whereas user might just want to load few files. Like for the file2 it returns location to 16 bundles, and user might just want to load 2 or 3 of them, not all 16.

'bundles',
'CST_L.trk')

return file1, file2

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

same comment as above

'hcp_tractogram',
'streamlines.trk')

return file1

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

same comment as above

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @BramshQamar, here the second part of the review. I think it is missing one file. Otherwise I need to test it on windows and it will be good for me

f3._data += np.array([100, 0, 0])

f.extend(f2)
f.extend(f3)

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

Can you create an init function? Having this global variable in this scope can be problematic later... Especially when pytest or nosetests grab all the functions.

This comment has been minimized.

@BramshQamar

BramshQamar Oct 8, 2018

Contributor

Hi Serge,

Should I create a class here? If I do so, this test file would be odd one as all other test files just have functions and no class.

x0 : string
rigid, similarity or affine transformation model (default affine)
rm_small_clusters : int

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

can you add , optional

Remove clusters that have less than `rm_small_clusters`
(default 50)
qbx_thr : variable int

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

same as above

qbx_thr : variable int
Thresholds for QuickBundlesX (default 15)
num_threads : int

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

same as above


BMD.setup(static, moving)
x0 = np.array([0, 0, 0, 0, 0, 0, 1., 1., 1, 0, 0, 0]) # affine
bmd_value = BMD.distance(x0.tolist())

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

I do not see any assert on this test function. It would be good if you checked whether the output files exist or not.

let's visualize atlas tractogram and target tractogram before registration
"""

interactive = True

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

can you set up interactive as False because it blocks the doc generation

let's visualize atlas tractogram and target tractogram after registration
"""

interactive = True

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

same comment as above

together
"""

interactive = True

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

same comment as above

bundle together
"""

interactive = True

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

same as above

@@ -61,5 +61,8 @@
viz_surfaces.py
viz_roi_contour.py
viz_ui.py
tractogram_registration.py

This comment has been minimized.

@skoudoro

skoudoro Sep 28, 2018

Member

where is this example? I do not see it on this repo?

self.nb_streamlines = len(self.streamlines)
self.verbose = verbose

self.start_thr = [40, 25, 20]
if rng is None:

This comment has been minimized.

@skoudoro

skoudoro Sep 29, 2018

Member

you can replace this if by self.rng = rng or np.random.RandomState()


if __name__ == '__main__':
npt.run_module_suite()
for i in range(20):

This comment has been minimized.

@skoudoro

skoudoro Sep 29, 2018

Member

What is the goal of this loop?

@@ -66,7 +66,7 @@
let's visualize atlas tractogram and target tractogram after registration
"""

interactive = True
interactive = false

This comment has been minimized.

@skoudoro

skoudoro Oct 9, 2018

Member

Typo: False instead of false

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Oct 11, 2018

Thank you @BramshQamar! Congratulations on your first large contribution to DIPY. And thanks all reviewers for their help too. :)

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Oct 11, 2018

All required fixes are done! This is merging .... ! :)

@Garyfallidis Garyfallidis merged commit 6e5c9fb into nipy:master Oct 11, 2018

2 of 4 checks passed

codecov/patch 84.26% of diff hit (target 87.34%)
Details
codecov/project 87.29% (-0.05%) compared to 1f0e5bf
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