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

FIX: PEP8 for segment #953

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@souravsingh
Contributor

souravsingh commented Mar 1, 2016

This PR fixes the scripts mentioned in #883 to some extent.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 1, 2016

LGTM

E.Garyfallidis, "Towards an accurate brain tractography", PhD thesis, 2012
E.Garyfallidis, "Towards an accurate brain tractography", PhD thesis, 2012

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

Not for this PR, but @Garyfallidis - there's probably a better citation for this, no?

This comment has been minimized.

This comment has been minimized.

@souravsingh

souravsingh Mar 1, 2016

Contributor

So, Should I update the Citation accordingly?

This comment has been minimized.

@MarcCote

MarcCote Mar 1, 2016

Contributor

I don't know, it's the old version which is deprecated.

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

No hurry on that one. If @Garyfallidis confirms, you can go ahead and do
that. Otherwise, I can merge this one, once you've had an opportunity to
address the other comments. Thanks!

On Tue, Mar 1, 2016 at 8:37 AM, Sourav Singh notifications@github.com
wrote:

In dipy/segment/quickbundles.py
#953 (comment):

     Citation
     ---------

- E.Garyfallidis, "Towards an accurate brain tractography", PhD thesis, 2012

  •    E.Garyfallidis, "Towards an accurate brain tractography", PhD thesis, 2012
    

So, Should I update the Citation accordingly?


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/953/files#r54594277.

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

Speaking of which -- any ideas on a strategy for removing support for this,
and expunging this code?

On Tue, Mar 1, 2016 at 8:41 AM, Marc-Alexandre Côté <
notifications@github.com> wrote:

In dipy/segment/quickbundles.py
#953 (comment):

     Citation
     ---------

- E.Garyfallidis, "Towards an accurate brain tractography", PhD thesis, 2012

  •    E.Garyfallidis, "Towards an accurate brain tractography", PhD thesis, 2012
    

I don't know, it's the old version which is deprecated.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/953/files#r54594874.

This comment has been minimized.

@MarcCote

MarcCote Mar 1, 2016

Contributor

The first thing would be to change bin/dipy_quickbundles to use the new QB :P. I was waiting for the workflows. I can give it a try tomorrow. Regading removing support, we already added a warning and it has been there for more than a year now. I do see other option beside simply removing the code.

This comment has been minimized.

@MarcCote

MarcCote Mar 5, 2016

Contributor

@arokem I talked with @Garyfallidis, he said to leave the citation to his Ph.D. thesis as we will remove this file at some point anyway (after the dipy_quickbundle script is modified).

This comment has been minimized.

@arokem

arokem Mar 5, 2016

Member

Right on. Still waiting for @souravsingh to address a few more comments here, before this can be merged.

assert_equal(cluster, clusters[-1])

assert_array_equal(list(itertools.chain(*clusters)), list(itertools.chain(*list_of_indices)))
assert_array_equal(

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

I think the following would be easier to read:

assert_array_equal(list(itertools.chain(*clusters)), 
                   list(itertools.chain(*list_of_indices)))

EDIT: comment edited to fix the indentation.

This comment has been minimized.

@arokem

arokem Mar 6, 2016

Member

Could you please change this one?

This comment has been minimized.

@arokem

arokem Mar 14, 2016

Member

This still needs to be changed

This comment has been minimized.

@arokem

arokem Mar 28, 2016

Member

This still needs to be changed into two lines.

This comment has been minimized.

@arokem

arokem Apr 1, 2016

Member

This still needs to be addressed

*clusters)),
list(
itertools.chain(
*list_of_indices)))

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

Same here

*clusters)),
list(
itertools.chain(
*list_of_indices)))

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

And here

1,
2 *
nb_clusters +
1)]

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

This line will be easier to read if you break it at the for:

indices = [rng.randint(0, 10, size=i) 
           for i in range(nb_clusters+1, 2*nb_clusters+1)]
@@ -511,11 +561,11 @@ def test_cluster_map_comparison_with_int():
def test_cluster_map_comparison_with_object():
nb_clusters = 4
cluster_map = ClusterMap()
#clusters = []
# clusters = []

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

I think this line can go away altogether

for i in range(nb_clusters):
new_cluster = Cluster(indices=range(i))
cluster_map.add_cluster(new_cluster)
#clusters.append(new_cluster)
# clusters.append(new_cluster)

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

This line is also apparently not needed.

*clusters.centroids)),
list(
itertools.chain(
*centroids)))

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

Same comment as before.

1:] -
streamline[
:-
1]) ** 2)))

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

How about:

np.sum(np.sqrt(np.sum((streamline[1:] - 
                       streamline[:-1]) ** 2)))

EDIT: comment edited to fix indentation

(streamline[
1:] -
streamline[
:-

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

Same comment as above. This is hard to read when it's broken into this many pieces.

np.cross(
rot_axis,
s_zero_mean),
axis=1) / norm(rot_axis)

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

How about:

        np.sqrt(2 * opposite ** 2 * 
             (1 - np.cos(60. * np.pi / 180.))).astype(dtype)

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

But with the second line properly indented to match indentation level :-)

This comment has been minimized.

@arokem

arokem Mar 14, 2016

Member

And this as well.

metric.are_compatible(
f1.shape,
f2.shape),
same_nb_points)

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

This could probably be two lines.

1, 2], [
3, 4, 5], [
6, 7, 8, 9], [
10, 11, 12, 13, 14]]

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

This is hard to read like this. How about:

clusters_truth = [[0], 
                          [1, 2], 
                          [3, 4, 5], 
                          [6, 7, 8, 9], 
                          [10, 11, 12, 13, 14]]

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

But with the indentation of the second line onwards correctly matched (it's a bit hard to do with the comment text boxes on Github :-D)

itertools.chain(
*subclusters)),
sorted(
cluster.indices))

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

This can be two lines

*clusters)),
list(
itertools.chain(
*clusters_truth)))

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

This can be two lines

@arokem

This comment has been minimized.

Member

arokem commented Mar 1, 2016

I had a few comments. In particular, in several places, you tend to break lines up into several lines. It would be good to make these breaks match logical pieces in the code. For example, a list of nested lists is best broken into the separate lists, instead of breaking in the middle of one of the nested lists. Thanks a lot for doing this!

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 1, 2016

Thanks a lot for the suggestions to the PR. I will work on updating the PR real quick.

@arokem

This comment has been minimized.

Member

arokem commented Mar 5, 2016

Any progress on these comments?

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 5, 2016

@arokem I have made updates for a few comments. I will be making a commit shortly.

souravsingh added some commits Mar 6, 2016

Small fixes for test_clustering.py
Fixes to the script as suggested by @arokem
Fixes to test_feature.py
Fixes to test_feature.py as suggested by @arokem
Fixes to test_metric.py
Fixes to test_metric.py as suggested by @arokem
Fixes to test_quickbundles.py
Fixes to the script as suggested by @arokem
@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 6, 2016

@arokem I have made changes in the scripts

assert_array_equal(list(itertools.chain(*clusters)), list(itertools.chain(*list_of_indices)))
assert_array_equal(
list(itertools.chain(*clusters)),
list(itertools.chain(*list_of_indices)))

This comment has been minimized.

@arokem

arokem Mar 6, 2016

Member

And here as well?

This comment has been minimized.

@arokem

arokem Mar 14, 2016

Member

And here as well.

This comment has been minimized.

@arokem

arokem Mar 28, 2016

Member

Here as well: change to two lines.

@@ -302,7 +323,8 @@ def test_cluster_map_remove_cluster():

clusters.remove_cluster(cluster2)
assert_equal(len(clusters), 2)
assert_array_equal(list(itertools.chain(*clusters)), list(itertools.chain(*[cluster1, cluster3])))
assert_array_equal(list(itertools.chain(*clusters)),
list(itertools.chain(*[cluster1, cluster3])))

This comment has been minimized.

@arokem

arokem Mar 6, 2016

Member

Like this! Thanks - this one is exactly the way the other ones about should also be.

assert_array_equal(cluster_map[advanced_indices], [clusters[i] for i in advanced_indices])
assert_array_equal(
cluster_map[advanced_indices], [
clusters[i] for i in advanced_indices])

This comment has been minimized.

@arokem

arokem Mar 6, 2016

Member

This is too many lines and broken up in a way that's hard to read. How about:

assert_array_equal(cluster_map[advanced_indices],
[clusters[i] for i in advanced_indices])

This comment has been minimized.

@arokem

arokem Mar 14, 2016

Member

And this too

This comment has been minimized.

@arokem

arokem Mar 28, 2016

Member

This one still needs to be adressed.

assert_arrays_equal(
cluster_map.get_large_clusters(
nb_clusters + 1),
large_clusters)

This comment has been minimized.

@arokem

arokem Mar 6, 2016

Member

Some here - these are all broken into way to many lines. It's impossible to read where one line ends and the other starts.

This comment has been minimized.

@arokem

arokem Mar 6, 2016

Member

This is a good place to break:

assert_equal(len(cluster_map.get_small_clusters(nb_clusters)),
len(small_clusters))

small_clusters = [Cluster(indices=indices[i]) for i in range(nb_clusters)]
cluster_map.add_cluster(*small_clusters)

# Randomly generate small clusters
indices = [rng.randint(0, 10, size=i) for i in range(nb_clusters+1, 2*nb_clusters+1)]
indices = [rng.randint(0,10,size=i)
for i in range(nb_clusters +1, 2*nb_clusters+1)]

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove leading space.

assert_equal(len(cluster_map.get_small_clusters(nb_clusters)),
len(small_clusters))
assert_arrays_equal(cluster_map.get_small_clusters(nb_clusters),
small_clusters)

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove leading space.

@@ -556,14 +576,15 @@ def test_cluster_map_centroid_add_cluster():
assert_equal(cluster, clusters[-1])

assert_equal(type(clusters.centroids), list)
assert_array_equal(list(itertools.chain(*clusters.centroids)), list(itertools.chain(*centroids)))
assert_array_equal(list(itertools.chain(*clusters.centroids)),
list(itertools.chain(*centroids)))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove leading space.

@@ -612,18 +634,21 @@ def test_cluster_map_centroid_iter():
clusters = []
for i in range(nb_clusters):
new_centroid = np.zeros_like(features)
new_cluster = ClusterCentroid(new_centroid, indices=rng.randint(0, len(data), size=10))
new_cluster = ClusterCentroid(new_centroid,
indices=rng.randint(0, len(data), size=10))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Requires some indentation.

assert_array_equal(cluster_map, clusters)
assert_array_equal(cluster_map.clusters, clusters)
assert_array_equal(cluster_map, [cluster.indices for cluster in clusters])

# Set refdata
cluster_map.refdata = data
assert_array_equal(cluster_map, [[data[i] for i in cluster.indices] for cluster in clusters])
assert_array_equal(
cluster_map, [[data[i] for i in cluster.indices] for cluster in clusters])

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

See previous comment.

@@ -645,11 +670,12 @@ def test_cluster_map_centroid_getitem():
assert_equal(cluster_map[i], clusters[i])

# Test advanced indexing
assert_array_equal(cluster_map[advanced_indices], [clusters[i] for i in advanced_indices])
assert_array_equal(cluster_map[advanced_indices],
[clusters[i] for i in advanced_indices])

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Needs indentation.

def cluster(self, data, ordering=None):
pass

clustering_algo = SubClustering()
assert_raises(NotImplementedError, super(SubClustering, clustering_algo).cluster, None)
assert_raises(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Too many lines. Modify to something like:

assert_raises(NotImplementedError,
              super(SubClustering, clustering_algo).cluster, None)
def __init__(self, nb_points):
super(ResampleFeature, self).__init__(is_order_invariant=False)
self.nb_points = nb_points
if nb_points <= 0:
raise ValueError("ResampleFeature: `nb_points` must be strictly positive: {0}".format(nb_points))
raise ValueError(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Use a msg variable.

for nb_points in [2, 5, 2*max_points]:
for feature in [dipymetric.ResampleFeature(nb_points), ResampleFeature(nb_points)]:
for nb_points in [2, 5, 2 * max_points]:
for feature in [

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Could save a line:

        for feature in [dipymetric.ResampleFeature(nb_points),
                        ResampleFeature(nb_points)]:
@@ -76,20 +81,23 @@ def extract(self, streamline):
# Test method extract
features = feature.extract(s)
assert_equal(features.shape, (nb_points, s.shape[1]))
assert_array_almost_equal(features, set_number_of_points(s, nb_points))
assert_array_almost_equal(features,

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove trailing space.


# This feature type is not order invariant
assert_false(feature.is_order_invariant)
for s in [s1, s2, s3, s4]:
features = feature.extract(s)
features_flip = feature.extract(s[::-1])
assert_array_equal(features_flip, set_number_of_points(s[::-1], nb_points))
assert_array_equal(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Reads better like:

                assert_array_equal(features_flip, 
                                   set_number_of_points(s[::-1], nb_points))
@@ -8,7 +8,8 @@

def norm(x, ord=None, axis=None):
if axis is not None:
return np.apply_along_axis(np.linalg.norm, axis, x.astype(np.float64), ord)
return np.apply_along_axis(np.linalg.norm, axis,
x.astype(np.float64), ord)

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove leading space.

opposite = norm(np.cross(rot_axis, s_zero_mean),
axis=1) / norm(rot_axis)
distances = np.sqrt(2 * opposite**2 *
(1 - np.cos(60. * np.pi / 180.))).astype(dtype)

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Indent with opening parenthesis of np.sqrt(

d = np.mean(distances)
assert_almost_equal(metric.dist(s, s_rotated), d, 5)

for s1, s2 in itertools.product(*[streamlines]*2): # All possible pairs
# Extract features since metric doesn't work directly on streamlines
for s1, s2 in itertools.product(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Put the comment above the for loop instead, like:

        # All possible pairs
        for s1, s2 in itertools.product(*[streamlines] * 2):
f1 = metric.feature.extract(s1)
f2 = metric.feature.extract(s2)

# Test method are_compatible
same_nb_points = f1.shape[0] == f2.shape[0]
assert_equal(metric.are_compatible(f1.shape, f2.shape), same_nb_points)
assert_equal(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

This can fit on two lines instead of three, pay attention to the indention though.

@@ -104,7 +115,8 @@ def dist(self, v1, v2):

# This metric type is order invariant
assert_true(metric.is_order_invariant)
for s1, s2 in itertools.product(*[streamlines]*2): # All possible pairs
for s1, s2 in itertools.product(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Like previous comment, move the comment above the for loop.

@@ -159,8 +172,10 @@ def dist(self, v1, v2):
assert_equal(metric.dist(v1, v2), 0.5) # orthogonal
assert_equal(metric.dist(v1, v3), 1.) # opposite

for s1, s2 in itertools.product(*[streamlines]*2): # All possible pairs
# Extract features since metric doesn't work directly on streamlines
for s1, s2 in itertools.product(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Like previous comment, move the comment above the for loop.

@@ -182,7 +197,8 @@ def dist(self, v1, v2):

# This metric type is not order invariant
assert_false(metric.is_order_invariant)
for s1, s2 in itertools.product(*[streamlines]*2): # All possible pairs
for s1, s2 in itertools.product(

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Like previous comment, move the comment above the for loop.

@@ -226,17 +242,19 @@ def test_distance_matrix():

for i in range(len(data)):
for j in range(len(data)):
assert_equal(D[i, j], dipymetric.dist(metric, data[i], data[j]))
assert_equal(D[i, j],
dipymetric.dist(metric, data[i], data[j]))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Needs a space.

D = dipymetric.distance_matrix(metric, data, data2)
assert_equal(D.shape, (len(data), len(data2)))

for i in range(len(data)):
for j in range(len(data2)):
assert_equal(D[i, j], dipymetric.dist(metric, data[i], data2[j]))
assert_equal(D[i, j],
dipymetric.dist(metric, data[i], data2[j]))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Needs a space.

@@ -226,17 +242,19 @@ def test_distance_matrix():

for i in range(len(data)):
for j in range(len(data)):
assert_equal(D[i, j], dipymetric.dist(metric, data[i], data[j]))
assert_equal(D[i, j],

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove trailing space.

D = dipymetric.distance_matrix(metric, data, data2)
assert_equal(D.shape, (len(data), len(data2)))

for i in range(len(data)):
for j in range(len(data2)):
assert_equal(D[i, j], dipymetric.dist(metric, data[i], data2[j]))
assert_equal(D[i, j],

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove trailing space.

@@ -72,7 +77,11 @@ def test_quickbundles_2D():
data += [rng.randn(1, 2) + np.array([-10, -10]) for i in range(5)]
data = np.array(data, dtype=dtype)

clusters_truth = [[0], [1, 2], [3, 4, 5], [6, 7, 8, 9], [10, 11, 12, 13, 14]]
clusters_truth = [[0],
[1, 2],

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Wrong indentation, please align with the first '['.

@@ -97,13 +106,16 @@ def test_quickbundles_2D():
# Find the corresponding cluster in 'clusters_truth'
for cluster_truth in clusters_truth:
if cluster_truth[0] in cluster.indices:
assert_equal(sorted(cluster.indices), sorted(cluster_truth))
assert_equal(sorted(cluster.indices),
sorted(cluster_truth))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Needs space

assert_equal(len(subclusters), len(cluster))
assert_equal(sorted(itertools.chain(*subclusters)), sorted(cluster.indices))
assert_equal(sorted(itertools.chain(*subclusters)),
sorted(cluster.indices))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Indentation, sorted should be aligned with the one above.

@@ -115,19 +127,21 @@ def test_quickbundles_2D():
clusters = quickbundles(data, metric, threshold=0)
assert_equal(len(clusters), len(data))
assert_array_equal(list(map(len, clusters)), np.ones(len(data)))
assert_array_equal([idx for cluster in clusters for idx in cluster.indices], range(len(data)))
assert_array_equal([idx for cluster in clusters for idx in cluster.indices],
range(len(data)))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove leading space.


clusters = qb.cluster(rdata)
# By default `refdata` refers to data being clustered.
assert_equal(clusters.refdata, rdata)
# Set `refdata` to return indices instead of actual data points.
clusters.refdata = None
assert_array_equal(list(itertools.chain(*clusters)), list(itertools.chain(*clusters_truth)))
assert_array_equal(list(itertools.chain(*clusters)),
list(itertools.chain(*clusters_truth)))

This comment has been minimized.

@MarcCote

MarcCote Apr 1, 2016

Contributor

Remove leading space.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Apr 1, 2016

I made some comments. Also, pay attention to the trailing spaces and the indentation.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jun 13, 2016

Since #1025 has been merged, this PR is now deprecated. I'm closing it. Thanks @souravsingh for your effort.

@MarcCote MarcCote closed this Jun 13, 2016

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