RF - Moved tracking.localtrack._local_tracker to DirectionGetter.generate_streamline. #2368
Conversation
|
Hello @gabknight, Thank you for updating ! Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated at 2021-05-07 15:48:22 UTC |
Codecov Report
@@ Coverage Diff @@
## master #2368 +/- ##
==========================================
+ Coverage 85.21% 85.24% +0.02%
==========================================
Files 125 125
Lines 16569 16581 +12
Branches 2686 2686
==========================================
+ Hits 14120 14134 +14
Misses 1762 1762
+ Partials 687 685 -2
|
gabknight
changed the title
|
Can you give me feedback on this? I moved the |
There was a problem hiding this comment.
Hi @gabknight,
See below some of my comments. I agree that generate_streamlines should be on DirectionGetter. we need to rename generate_streamlines from now. it is really confusing with the one in tracking.py
Overall, it looks good to me.
gabknight
force-pushed
the
RF_tracking_framework
branch
2 times, most recently
from
165a3cd
to
723b9f3
Compare
|
@skoudoro thx for the feedback. I believe all is fixed. I also suggested changing I don't need to add deprecated warnings for internal function changes, do I? |
There was a problem hiding this comment.
Hi @gabknight,
Thank you for the update.
I like the strategy to rename _generate_streamlines(.) to _generate_tractogram(.) in dipy.tracking.local_tracking.py. it makes more sense to me.
Apart from that, there are some functions in closest_peaks not used so not necessary?
I don't need to add deprecated warnings for internal function changes, do I?
No you don't, no needs
| cdef extern from "dpy_math.h" nogil: | ||
| int dpy_signbit(double x) | ||
| double dpy_rint(double x) | ||
| double fabs(double) | ||
|
|
||
| @cython.cdivision(True) | ||
| cdef inline double stepsize(double point, double increment) nogil: | ||
| """Compute the step size to the closest boundary in units of increment.""" | ||
| cdef: | ||
| double dist | ||
| dist = dpy_rint(point) + .5 - dpy_signbit(increment) - point | ||
| if dist == 0: | ||
| # Point is on an edge, return step size to next edge. This is most | ||
| # likely to come up if overstep is set to 0. | ||
| return 1. / fabs(increment) | ||
| else: | ||
| return dist / increment | ||
|
|
||
| cdef void step_to_boundary(double * point, double * direction, | ||
| double overstep) nogil: | ||
| """Takes a step from point in along direction just past a voxel boundary. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| direction : c-pointer to double[3] | ||
| The direction along which the step should be taken. | ||
| point : c-pointer to double[3] | ||
| The tracking point which will be updated by this function. | ||
| overstep : double | ||
| It's often useful to have the points of a streamline lie inside of a | ||
| voxel instead of having them lie on the boundary. For this reason, | ||
| each step will overshoot the boundary by ``overstep * direction``. | ||
| This should not be negative. | ||
|
|
||
| """ | ||
| cdef: | ||
| double step_sizes[3] | ||
| double smallest_step | ||
|
|
||
| for i in range(3): | ||
| step_sizes[i] = stepsize(point[i], direction[i]) | ||
|
|
||
| smallest_step = step_sizes[0] | ||
| for i in range(1, 3): | ||
| if step_sizes[i] < smallest_step: | ||
| smallest_step = step_sizes[i] | ||
|
|
||
| smallest_step += overstep | ||
| for i in range(3): | ||
| point[i] += smallest_step * direction[i] | ||
|
|
||
| cdef void fixed_step(double * point, double * direction, double step_size) nogil: | ||
| """Updates point by stepping in direction. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| direction : c-pointer to double[3] | ||
| The direction along which the step should be taken. | ||
| point : c-pointer to double[3] | ||
| The tracking point which will be updated by this function. | ||
| step_size : double | ||
| The size of step in units of direction. | ||
|
|
||
| """ | ||
| for i in range(3): | ||
| point[i] += direction[i] * step_size | ||
|
|
There was a problem hiding this comment.
I do not understand why you need this block in close_peak_direction_getter.pyx. it is not used.
There was a problem hiding this comment.
we don't need it. This should have been removed when we moved generate_streamline(.) to the parent class DirectionGetter. thx.
There was a problem hiding this comment.
Ok, Great! This PR is ready for me.
There are 2-3 things that can be improved (unnecessary copy) but we can do that in a new PR.
|
I think this PR is ready to go! I will wait until tomorrow evening for additional comments. Feel free to review this PR! |
|
Thank you @gabknight! |
This is the first PR to generalized DIPY's tracking framework.
This PR moved the code from
dipy.tracking.localtrack._local_tracker()to theDirectionGetter.generate_streamline(). This will allow forDirectionGettersto override how streamlines are generated in the local tracking framework. DIPY'sDirectionGettersselect the next tracking based on the current position and previous direction until a stopping criterion is reached. This modification will allow newDirectionGettersto use, e.g., the whole sequence of previous directions and positions to select the next tracking direction.This doesn't have any effect on the generated streamlines or interfaces.
The
ParticleFileteringTrackingis not affected by the change. Future PR should modifyPFTto leave streamline generation toDirectionGettersin a similar fashion, such that it can benefit from future overridingDirectionGetter.generate_streamline.