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 - Bootstrap Direction Getter (cythonized) #1431

Merged
merged 51 commits into from Apr 29, 2018

Conversation

Projects
7 participants
@gabknight
Contributor

gabknight commented Feb 14, 2018

This follows PR #1184.

MrBago and others added some commits Dec 16, 2016

@gabknight

This comment has been minimized.

Contributor

gabknight commented Feb 16, 2018

Thanks @kesshijordan

@Garyfallidis, @MrBago, @skoudoro This is ready for review :)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 6, 2018

Hello @gabknight ! Please rebase :)

@gabknight gabknight changed the base branch from master to rename-contributing Mar 15, 2018

@gabknight gabknight changed the base branch from rename-contributing to master Mar 15, 2018

@gabknight

This comment has been minimized.

Contributor

gabknight commented Mar 15, 2018

OK. I had to do a merge here, too many conflicts as the first commits on this PR are very old.

@gabknight

This comment has been minimized.

Contributor

gabknight commented Mar 16, 2018

@skoudoro, Do you know what is this error:

Collecting dill>=0.2.6 (from multiprocess->cvxpy)
THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
    h5py from https://7933911d6844c6c53a7d-47bd50c35cd79bd838daf386af554a83.ssl.cf2.rackcdn.com/h5py-2.8.0rc1.post0-cp35-cp35m-manylinux1_x86_64.whl#sha256=7d43d155304a752e5354c8eba6785657c76ca715a7aa2034cb930b23615181ca:
        Expected sha256 7d43d155304a752e5354c8eba6785657c76ca715a7aa2034cb930b23615181ca
             Got        2d4458e7a8b20a7dcc447e51e4870feb3b6e4785d1fd5c1eb7009f783dddf829
@skoudoro

This comment has been minimized.

Member

skoudoro commented Apr 19, 2018

Can you have a look at this PR @nilgoyette @jchoude? Thanks!

@nilgoyette

I didn't run anything and I don't plan to. I simply checked the code and it's as good as cython can be. I can't actually complain because I started this cython crazyness :D

You can safely ignore this paragraph. It's not so much related to this PR, it's more of a general comment. I doubt cythonizing everything is a good idea. I don't know how much of DiPy is getting cythonized, but I know the % is increasing. We/you choosed Python because it's a super readable language with bateries included where you can code something quickly, then cython slowly creeps its way into the project. It's odd.

count = 0
while count < self.max_attempts:
count += 1

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

If you're going to increment it by 1 each time, you might as well use a normal for loop.

This comment has been minimized.

@gabknight

gabknight Apr 23, 2018

Contributor

done. thx.

cdef class ClosestPeakDirectionGetter(PmfGenDirectionGetter):
"""A direction getter that returns the closest odf peak to previous tracking
direction.
"""

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

Indentation

hsph_updated = HemiSphere.from_sphere(unit_icosahedron).subdivide(2)
vertices = hsph_updated.vertices
bvecs = vertices
bvals = np.ones(len(vertices)) * 1000

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

numpy.full I know speed in not important because it's a test, but I think it's a good function to know when you want something else than 0 or 1.

This comment has been minimized.

@gabknight

gabknight Apr 23, 2018

Contributor

thx!

def test_bdg_get_direction():
"""This tests that the new tracking direction if founded.

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

I don't understand this sentence

peak_dirs : array (N, 3)
N unit vectors.
direction : array (3,) or None
Previous direction. The new direction is save here.

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

saveD

direction[1] = peak_dirs[closest_peak_i, 1]
direction[2] = peak_dirs[closest_peak_i, 2]
return 0
elif closest_peak_dot <= -cos_similarity and closest_peak_i >= 0:

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

You have 2 times closest_peak_i >= 0, I think you should refactor.

if trilinear_interpolate4d_c(self.data, point, self.vox_data) != 0:
for i in range(len_pmf):
self.pmf[i] = 0.0

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

Do you think it would be a good idea to have inlined helper functions, like clear_pmf, or threshold_pmf (0 of too low), etc.? I remember that copy_point helped readability a lot, maybe those small functions could also help. I don't know, it's a real question :)

This comment has been minimized.

@gabknight

gabknight Apr 23, 2018

Contributor

I did the change. It removed many repeated lines. Clearer, shorter, better. thx

B, m, n = shm.real_sym_sh_basis(6, theta, phi)
shm_coeff = np.random.random(B.shape[1])
# sphere_func is samples of the spherical function for each point of

This comment has been minimized.

@nilgoyette

nilgoyette Apr 20, 2018

Contributor

SampleD?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Apr 20, 2018

Thanks for the review @nilgoyette .
Concerning the cython percentage increase, I am not worried. As suggested before, we just need a Cython coding style and respect it. The fact is people pay a little less attention when they are doing a cython file which make the code less readable. Cython is really readable and we are in charge to make it happen.

Otherwise, everything runs like expected @gabknight @kesshijordan on my environment (windows 10 -python 3), Thanks so much for that!

@gabknight

This comment has been minimized.

Contributor

gabknight commented Apr 23, 2018

thx for the feedback @nilgoyette @skoudoro. All good on my side.

for count in range(self.max_attempts):
pmf = self._get_pmf(point)
_len = pmf.shape[0]

This comment has been minimized.

@nilgoyette

nilgoyette Apr 23, 2018

Contributor

So, why do you need _len? And while we're at it, i and/or count?

This comment has been minimized.

@gabknight

gabknight Apr 25, 2018

Contributor

No needs. Thanks for pointing out.

if closest_peak_dot <= -cos_similarity:
direction[0] = -peak_dirs[closest_peak_i, 0]
direction[1] = -peak_dirs[closest_peak_i, 1]
direction[2] = -peak_dirs[closest_peak_i, 2]

This comment has been minimized.

@nilgoyette

nilgoyette Apr 23, 2018

Contributor

Would it be possible to use copy_point on the top block? copy_point(direction, peak_dirs[closest_peak_i] would be clrearer. There's some other places like direction[0] = newdir[0] ... below too. Since the pattern arises often, maybe it would be better to create a copy_inverse_point or copy_mirror_point or something like that?

This comment has been minimized.

@gabknight

gabknight Apr 25, 2018

Contributor

yes. I made the change. I moved the copy_point function from localtracking to fast_numpy.py. Thanks for the comment.

self.__clear_pmf()
else:
self.vox_data[self.dwi_mask] = shm.bootstrap_data_voxel(
self.vox_data[self.dwi_mask], self.H, self.R)

This comment has been minimized.

@nilgoyette

nilgoyette Apr 23, 2018

Contributor

Indentation

This comment has been minimized.

@gabknight

gabknight Apr 25, 2018

Contributor

fixed. thx.

cdf = self._adj_matrix[
(direction[0], direction[1], direction[2])] * pmf
bool_array = self._adj_matrix[
(direction[0], direction[1], direction[2])]

This comment has been minimized.

@nilgoyette

nilgoyette Apr 23, 2018

Contributor

I don't know anything about this algo, but if your goal was to port it to Cython, there's a difference here. You don't multiply by the pmf anymore. Is it deliberate?

This comment has been minimized.

@gabknight

gabknight Apr 25, 2018

Contributor

yes, bellow (line 180) we used the pmf directly instead of the cdf (self._adj_matrix is 1s and 0s).

@nilgoyette

I could probably check more and more but I believe it's a good job overall (code only, I didn't run anything). Thank you for doing this and thank you for actually listening to the reviewers advices :) Good Job!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 29, 2018

Okay will merge this because this work has been waiting for years and it seems working well. However, we need to correct many issues after this PR is merged. It needs more documentation, more docstrings and a better and clear tutorial. The current tutorial does not really explain the difference between using bootstrap and not. Also we need a plan for refactoring tracking. Let's call this a summer of tracking? @kesshijordan let's talk about this, in person this week. Thank you all for all these extreme amount of work! There is more to do. But good progress so far.

@Garyfallidis Garyfallidis merged commit e58b28e into nipy:master Apr 29, 2018

3 checks passed

codecov/patch 94.64% of diff hit (target 87.49%)
Details
codecov/project 87.54% (+0.04%) compared to f6af802
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Dipy 0.14.0 automation moved this from Needs a review to Done Apr 29, 2018

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

Merge pull request nipy#1431 from gabknight/NF_bootstrap_direction_ge…
…tter_RF

NF - Bootstrap Direction Getter (cythonized)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment