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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

QuickBundlesX #1434

Merged
merged 36 commits into from Mar 3, 2018

Conversation

Projects
None yet
4 participants
@Garyfallidis
Member

Garyfallidis commented Feb 16, 2018

This PR is replacing #1380
This PR includes the fastest algorithm ever for clustering tractograms!!! 馃敟

Garyfallidis E. et al. QuickBundlesX: Sequential clustering of millions of streamlines in multiple levels of detail at record execution time. Proceedings of the, International Society of Magnetic Resonance in Medicine (ISMRM). Singapore, 4187, 2016.

@Garyfallidis Garyfallidis referenced this pull request Feb 16, 2018

Closed

WIP: QuickBundlesX #1380

@Garyfallidis Garyfallidis requested a review from MarcCote Feb 16, 2018

@MarcCote

In theory, QuickBundles(threshold) and QuickBundlesX(thresholds=[threshold]) should produce the same kind of results. Maybe we could refactor QuickBundles such that it uses the same Cython algorithm/code as QuickBundlesX (or maybe in a future PR). In any cases, we add some simple tests that check if the results are similar between QuickBundles(threshold) and QuickBundlesX(thresholds=[threshold]).

thresholds=self.thresholds,
ordering=ordering)
#cluster_map.refdata = streamlines

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

Remove comment.

DEF BIGGEST_FLOAT = 3.4028235e+38 # np.finfo('f4').max
DEF SMALLEST_FLOAT = -3.4028235e+38 # np.finfo('f4').max
THRESHOLD_MULTIPLIER = 2.

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

Unused.

@@ -99,7 +402,7 @@ cdef class ClustersCentroid(Clusters):
raise ValueError("'centroid_shape' must be a tuple or a int.")
self._centroid_shape = tuple2shape(centroid_shape)
# self.aabb

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

Remove this line.

@@ -175,6 +478,10 @@ cdef class ClustersCentroid(Clusters):
converged &= fabs(centroid[n, d] - updated_centroid[n, d]) < self.eps
centroid[n, d] = updated_centroid[n, d]
#cdef float * aabb = &self.centroids[id_cluster].aabb[0]

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

Remove this line.

self.metric = metric
self.features_shape = tuple2shape(features_shape)
self.threshold = threshold
self.max_nb_clusters = max_nb_clusters
self.clusters = ClustersCentroid(features_shape)
self.features = np.empty(features_shape, dtype=DTYPE)
self.features_flip = np.empty(features_shape, dtype=DTYPE)
self.bvh = bvh

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

Nowhere in the code, I could find Quickbundles instance with bvh=1. Maybe we should remove this functionality for now? Otherwise, we could do like for QuickbundlesX and always perform aabb checks?

return self._build_clustermap()
def evaluate_aabbb_checks():

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

I couldn't find a place where this function is used.

#test_show_qbx()
#test_3D_segments()
test_3D_points()

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

Newline missing?

thresholds = [4, 2, 1]
qbx_class = QuickBundlesX(thresholds)
qbx = qbx_class.cluster(points)
print(qbx)

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

What do we expect? Add assertions.

thresholds = [4, 2, 1]
qbx_class = QuickBundlesX(thresholds)
qbx = qbx_class.cluster(points)
print(qbx)

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

What do we expect? Add assertions.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 18, 2018

Codecov Report

Merging #1434 into master will increase coverage by 0.02%.
The diff coverage is 91.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1434      +/-   ##
==========================================
+ Coverage   87.38%   87.41%   +0.02%     
==========================================
  Files         237      238       +1     
  Lines       30069    30282     +213     
  Branches     3232     3253      +21     
==========================================
+ Hits        26277    26472     +195     
- Misses       3043     3057      +14     
- Partials      749      753       +4
Impacted Files Coverage 螖
dipy/segment/tests/test_quickbundles.py 98.31% <100%> (+0.36%) 猬嗭笍
dipy/segment/clustering.py 93.51% <79.41%> (-6.49%) 猬囷笍
dipy/segment/tests/test_qbx.py 96.77% <96.77%> (酶)

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 95a9cca...f81a4da. Read the comment docs.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 18, 2018

Hey @MarcCote I agree on the bvh. Needs some investigation. Any clues on the travis error? Strangely only one bot is failing https://travis-ci.org/nipy/dipy/jobs/343108569

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 19, 2018

The last bot is failing because of the new version of Cython. Look at the issue #1435

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 20, 2018

@MarcCote there is a change in Cython that will require some large RF of the code. Cython stopped supporting memory views in structs.

skoudoro and others added some commits Feb 28, 2018

@@ -104,3 +111,46 @@ cdef int same_shape(Shape shape1, Shape shape2) nogil:
same_shape &= shape1.dims[i] == shape2.dims[i]
return same_shape
cdef Data2D* create_memview(Py_ssize_t buffer_size, Py_ssize_t dims[MAX_NDIM]) nogil:

This comment has been minimized.

@MarcCote

MarcCote Mar 1, 2018

Contributor

Do we only want to support 2D memview? If so, we could rename the function so that it is clearer it is only for 2D array.

This comment has been minimized.

@skoudoro

skoudoro Mar 1, 2018

Member

ok, you are right, let's do it more generic

This comment has been minimized.

@skoudoro

skoudoro Mar 1, 2018

Member

Finally, I choose the easy path: renaming. The second option needs more thinking and we need to move forward with the release

"""
free(&(memview[0][0, 0]))
memview[0] = None # Necessary to decrease refcount
free(memview)

This comment has been minimized.

@MarcCote

MarcCote Mar 1, 2018

Contributor

Newline missing.

This comment has been minimized.

@skoudoro

skoudoro Mar 1, 2018

Member

Thanks

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 1, 2018

@MarcCote I addressed all your comments. If Travis agrees do merge this PR so that the other PRs can can start becoming green.

MarcCote and others added some commits Mar 2, 2018

Merge pull request #53 from MarcCote/rb_qbx2
Reducing code complexity
@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 3, 2018

Yeah! Looks good and all tests are happy. I think it is ready to merge, can you have a last look @MarcCote? Thanks to you and @Garyfallidis!

@MarcCote

After that, LGTM.

def test_circle_parallel_fornix():
circle = streamlines_in_circle(100, step_size=2)

This comment has been minimized.

@MarcCote

MarcCote Mar 3, 2018

Contributor

PEP8 - too many empty lines.

self.level = levels[level]
self.traverse_postorder(self.root, self._fetch_level)
return self.clusters
# cdef void _fetch_level(self, CentroidNode* node):

This comment has been minimized.

@MarcCote

MarcCote Mar 3, 2018

Contributor

let's delete those commented lines if everyone is happy with manipulating the TreeClusterMap python object instead of the QuickBundlesX Cython object.

@@ -110,5 +110,5 @@ cdef class QuickBundlesX(object):
cpdef object insert(self, Data2D datum, int datum_idx)
cdef void traverse_postorder(self, CentroidNode* node, void (*visit)(QuickBundlesX, CentroidNode*))
cdef void _dealloc_node(self, CentroidNode* node)
cdef void _fetch_level(self, CentroidNode* node)
# cdef void _fetch_level(self, CentroidNode* node)

This comment has been minimized.

@MarcCote

MarcCote Mar 3, 2018

Contributor

delete

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 3, 2018

Done @MarcCote !

@MarcCote MarcCote merged commit 5a8ed85 into nipy:master Mar 3, 2018

3 checks passed

codecov/patch 91.54% of diff hit (target 87.38%)
Details
codecov/project 87.41% (+0.02%) compared to 95a9cca
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 3, 2018

Let's make all the PRs green again!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 3, 2018

Super! @skoudoro please inform everyone to rebase their PRs. We can go green again :) Thank you both for all the help. That was a fast recovery with a sudden Cythonic change! Grrr.....

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

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