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

Select streamlines based on logical operations on ROIs #635

Merged
merged 51 commits into from Aug 24, 2015

Conversation

Projects
None yet
7 participants
@arokem
Member

arokem commented Apr 24, 2015

This PR starts what will be a long series of PRs towards the implementation of the AFQ system in dipy (see http://journals.plos.org/plosone/article?id=10.1371/journal.pone.0049790), work that I will undertake in close collaboration with @jyeatman, who has done much of the development of the system in Matlab, and has been the maintainer of that software for several years.

For now, this just does a couple of really simple things, as you can see:

  • Determines whether streamlines pass within some distance from a mask (a region of interest)
  • Combines the conclusions from this determination based on inclusion and exclusion criteria. For now, only a fairly limited set of logical operations are possible.

Questions include:

  • Does the select_by_roi function makes sense where it currently is?
  • Would we want/need to build out a more extensive set of logical operations over combinations of ROIs? What would that entail?
x_roi_coords = apply_affine(affine, roi_coords)
out = np.zeros(len(streamlines), dtype=bool)
for ii, sl in enumerate(streamlines):
for coord in sl:

This comment has been minimized.

@Garyfallidis

Garyfallidis May 1, 2015

Member

Hey bro @arokem! This looks very inefficient. And it is important that we can have this roi-filter functions as fast as possible. So, can I suggest that you write your "first" Cython code to increase the speed of this function? It would be great way to increase your Cython expertise too. Are you in for the challenge?

This comment has been minimized.

@arokem

arokem May 1, 2015

Member

Is "first" in quotes because it's supposed to be ironic?

Sure - I can try rewriting this in cython and testing to see how much of a win that would really be. I am worried that our code-base has a lot of cython, which is always a bit harder to maintain than python code, but it's certainly not going to be the largest piece of cython we have.

This comment has been minimized.

@Garyfallidis

Garyfallidis May 1, 2015

Member

The "first" is in quotes because maybe you have already written many cythonized functions and I am not aware of it. Definitely not ironic. This is a critical functionality and it will be a bottleneck if we are not smart. I think Cythonizing those intersections with ROIs is a great idea and will not add any problems on maintaining the code. These are small functions. If you see that the speed improvement is not considerable then we talk again. But I think you will be surprised from the speedup.

This comment has been minimized.

@arokem

arokem May 2, 2015

Member

OK - I've started this process, and will provide some more detailed profiling information down the line. I am happy to get feedback on what could be done in the cython code to speed things up even further.

This comment has been minimized.

@arokem

arokem May 2, 2015

Member

What I have done so far seems to have resulted in only a very small speed-up. I am using the fornix bundle as streamlines:

https://gist.github.com/arokem/51239fe55c07da101633

This level of cythonization takes me from about 730 (only python) to about 650 msec (python + cython), so about 10% speedup

Happy to hear what else should be done in the cython code to really take advantage of it, and increase the speedup more.

This comment has been minimized.

@jyeatman

jyeatman May 3, 2015

Hi Ariel,

A use case to try is a huge ROI. For example take the fornix bundle (or a
few thousand random streamlines) and perform an operation using a gray
matter mask as an ROI. This is the case that will typically bog down the
computation and would hopefully show a substantial speed up.

Jason D. Yeatman, PhD
Research Scientist
Institute for Learning and Brain Sciences (ILABS)
University of Washington
http://jasonyeatman.com

On Sat, May 2, 2015 at 1:30 PM, Ariel Rokem notifications@github.com
wrote:

In dipy/tracking/utils.py
#635 (comment):

  •    Distance (in the units of the streamlines, usually mm). If any
    
  •    coordinate in the streamline is within this distance from any voxel in
    
  •    the ROI, the filtering criterion is set to True for this streamlin,
    
  •    otherwise False. Default: 0
    
  • Returns

  • Boolean array. Set to True in each streamline
  • """
  • if affine is None:
  •    affine = np.eye(4)
    
  • roi_coords = np.array(np.where(target_mask)).T
  • x_roi_coords = apply_affine(affine, roi_coords)
  • out = np.zeros(len(streamlines), dtype=bool)
  • for ii, sl in enumerate(streamlines):
  •    for coord in sl:
    

What I have done so far seems to have resulted in only a very small
speed-up. I am using the fornix bundle as streamlines:

https://gist.github.com/arokem/51239fe55c07da101633

This level of cythonization takes me from about 730 (only python) to about
650 msec (python + cython), so about 10% speedup

Happy to hear what else should be done in the cython code to really take
advantage of it, and increase the speedup more.


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

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 1, 2015

Before I get into the details of the code, is it right that selecting on a list on includes is the same as selecting on the or of all those include and the same on the exclude? In other words are A and B below always the same if in_* and ex_* are boolean arrays?

include = [in_a, in_b, in_c]
exclude = [ex_a, ex_b, ex_c]
A = select(streamlines, include + exclude, [1, 1, 1, 0, 0, 0])

in_all = reduce(operator.or_, include)
ex_all = reduce(operator.or_, exclude)
B = select(streamlines, [in_all, ex_all], [1, 0])
  • fixed order of include array on B = ... line.
@MrBago

This comment has been minimized.

Contributor

MrBago commented May 6, 2015

I have 3 main things I would change about your PR:

  1. I think that the problem of selecting on multiple ROIs can be reduced to selecting on a pair of ROIs, an include and an exclude. I think it would make the code easier to follow if we had a first function which took an iterable of streamlines, an include ROI and an exclude ROI and preformed the selection. I think we should also have a second function that takes a list of ROIs and either calls the first function (or maybe just returns a pair of "aggregate ROIs" which can be passed to the first, but I like this version less).

  2. In terms of performance, comparing every point in every streamline to every point in every ROI is quite computationally expensive, O(P * N) where P is the number of points in all the streamlines and N is the total size of all the ROIs. I believe what we can do here is apply a binary_dilation (scipy.ndimage) to the input ROIs (I think the structure should be a circle with the appropriate radius). Then for each point, we just need to convert to voxel coordinates and look up a single point in the input ROI. The complexity here is roughly O(P + N) and I believe the overhead of the dilation will be very small compared to the benefit, but I'd like to profile it to know for sure.

  3. So far in dipy, we've been using generators for streamlines. That is any function that returns streamlines has been returning a generator and any function that accepts streamlines as inputs has been designed and tested to work with generator inputs. I'm not opposed to changing this on principle, but I think selection should actually work very nicely with the generator model. The idea with the generators is that if I want to do the following and my file is very large, ~8GB, the streamlines will be fed into the final consumer (density_map in this case) 1 by 1 and the streamlines will never all be loaded into memory all at once. Here is the example:

streamlines = read_trk_file(filename, as_generator=True)
subset_streamlines = select(streamlines, ...)
dm = density_map(subset_streamlines, shape, affine)

Dipy's tracking files currently return generators and nibable's read function optionally returns a generator. Typically, any function that accepts generators as an argument can instead be passed a list without any changes. Also, any function which returns a generator can be wrapped with list, something like out = list(select(...)), in order to force the output to be a list.

tol: float
Distance (in the units of the streamlines, usually mm). If any
coordinate in the streamline is within this distance from any voxel in
the ROI, the filtering criterion is set to True for this streamlin,

This comment has been minimized.

@jchoude

jchoude May 6, 2015

Contributor

Typo: streamlin -> streamline

@jchoude

This comment has been minimized.

Contributor

jchoude commented May 6, 2015

I just wanted to quickly comment that I agree with @MrBago 's suggestion in point 2). This is what we do in internal tools, and if the dilation radius is correctly set, the result should be equivalent.

@arokem

This comment has been minimized.

Member

arokem commented May 6, 2015

Thanks for the comments @MrBago! I have no quibbles with points 1 and 3. I thought about number 2 as well, but was thinking in the direction of a spatial smoothing, and distance tolerance implemented through thresholding on the smoothed image. I agree that the order of the computation is more favorable, but not sure how this bears out in practice, with the data we usually analyze (P>>N, typically), so profiling would be good.

@jchoude - could you share the code you were referring to? Would be useful to see what goes into setting the radius correctly.

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 6, 2015

@arokem I believe dialation is very close to what you have in this PR. Spacial smoothing might work, but it is going to be different than what you have here. As an example take a U shaped ROI, after smoothing the interior of the U will have higher intensity than the exterior at the same nominal distance.

Dialation using a cirlce structure returns all the regions within some radius of at least 1 true voxel in the input ROI, isn't that what you're going for?

A 1D array of boolean marking inclusion or exclusion criteria. If a
streamline is near any of the inclusion ROIs, it should evaluate to
True, unless it is also near any of the exclusion ROIs.

This comment has been minimized.

@jchoude

jchoude May 7, 2015

Contributor

Missing doc for affine and tol

@jchoude

This comment has been minimized.

Contributor

jchoude commented May 7, 2015

@arokem Sadly, I think I initially read too quickly. I re-read what you're trying to do, and we don't have code that does exactly that. My bad, sorry for the noise.

OTOH, going through your code has raised a question in my head: if you set your tol parameter to 0, would you expect a streamline which has at least one point inside one of the voxel of the ROI to still be selected? As for myself, I would say yes. If you disagree, the following may not hold true.

When you transform the voxel coordinates (in your near_roi function) back to world coordinates, you effectively get the coordinates of the center of those voxels in world space. Then, when you compute the distance (in _near_roi), you compute the point to point distance. In that case, there is no explicit consideration of the voxel size.

That means that, if we set the tol to 0, then there must be at least one point of the streamline overlapping with the center of a voxel, else the streamline won't be selected. However, this goes against the intuition that there only needs to be a point of the streamline inside a voxel to be selected...

If you want to have this behavior (full voxel selection), that means that either
1- the tol parameter needs to be better documented to explicitly say that the voxel size should be included
2- the voxel size should be internally used in the distance threshold
3- an approach based on something derived from the target function might be better suited to this task.

Personally, I don't like 1 and 2. For 1, it might be confusing for users. For 2, it may be complex to handle the case where voxels are anisotropic.

Maybe having an approach based on the target function, and possibly a supersampled grid and mask dilation might be another way to go.

I know that this is a long comment (probably my longest up to today ;)). If it's not clear, please let me know. If you disagree, disregard this :)

@arokem

This comment has been minimized.

Member

arokem commented May 8, 2015

I am happy that I managed to get your interest up enough to review this PR.
I do appreciate the thought everyone is putting into this. It is going to
be an important piece in something that we hope to use a lot, so it's
important to get it right: conceptually, and in terms of implementation.

Concerning the tol kwarg: I actually think that the most natural units
for this are not in voxel size, but in mm. I agree that the default value
is somewhat misleading, because we will probably never use this with a
tolerance of 0. How about setting this per default to None and then
extracting voxel size from the affine, and including things that are within
the voxels of the ROI? The simplest would be to treat voxels as though they
are spheres with some radius for this purpose. Otherwise, for simplicity,
we can also just set this to some 'typical' number of mm, e.g. 1 mm.

Concerning dilation: I am not sure this would do what we want, unless we
resample the ROI to a fine resolution, that depends somehow on the
tolerance value. Imagine for example that you want to include streamlines
that have nodes that are <=1.5 voxel-size units away from the center of a
voxel, but not those streamlines passing at 1.6 voxel units away. You would
have to somehow resample the ROI to 0.1 of the original size and proceed to
dilate by 5 steps out from the edges of the ROI.

On Thu, May 7, 2015 at 6:15 AM, Jean-Christophe Houde <
notifications@github.com> wrote:

@arokem https://github.com/arokem Sadly, I think I initially read too
quickly. I re-read what you're trying to do, and we don't have code that
does exactly that. My bad, sorry for the noise.

OTOH, going through your code has raised a question in my head: if you set
your tol parameter to 0, would you expect a streamline which has at least
one point inside one of the voxel of the ROI to still be selected? As for
myself, I would say yes. If you disagree, the following may not hold true.

When you transform the voxel coordinates (in your near_roi function) back
to world coordinates, you effectively get the coordinates of the center of
those voxels in world space. Then, when you compute the distance (in
_near_roi), you compute the point to point distance. In that case, there
is no explicit consideration of the voxel size.

That means that, if we set the tol to 0, then there must be at least one
point of the streamline overlapping with the center of a voxel, else the
streamline won't be selected. However, this goes against the intuition that
there only needs to be a point of the streamline inside a voxel to be
selected...

If you want to have this behavior (full voxel selection), that means that
either
1- the tol parameter needs to be better documented to explicitly say that
the voxel size should be included
2- the voxel size should be internally used in the distance threshold
3- an approach based on something derived from the target function might
be better suited to this task.

Personally, I don't like 1 and 2. For 1, it might be confusing for users.
For 2, it may be complex to handle the case where voxels are anisotropic.

Maybe having an approach based on the target function, and possibly a
supersampled grid and mask dilation might be another way to go.

I know that this is a long comment (probably my longest up to today ;)).
If it's not clear, please let me know. If you disagree, disregard this :)


Reply to this email directly or view it on GitHub
#635 (comment).

@jchoude

This comment has been minimized.

Contributor

jchoude commented May 8, 2015

Hey @arokem

ok, now I clearly understand your definition of the tol arg. In that case, it needs to be clearly documented, and a default value as you are suggesting would be interesting.

As for the strategy of considering the voxels as spheres with a radius, that would fit perfectly with your current code. I guess at that point that's more of a "philosophical" choice... Personally, I know that if I have a ROI taken from either an automatic segmentation or drawn in any software, and I load it in a visualization tool and check the streamlines that are selected by my ROI, I would expect to get all streamlines going through voxels included in my ROI. Using a sphere with a radius + the tolerance would get almost the same result, with possibly some false positives or negatives.

At the point, it's mostly a question of perfect precision versus efficiency. I'll let you make the call 😃

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 8, 2015

@arokem I understand your concern and see some of the advantages of your current definition. I believe both definitions have some issues. For example if you think of voxels as spheres, and set that diameter to be the edge length of one voxel, you'll end up with a grid of non-overlapping spheres and you'll miss points that lie in the space between these spheres. The result is that your ROI will have internal gaps. To avoid gaps in a 3D ROI you'll need to set tol to be >= sqrt(sum((voxel_edges / 2)**2)).

The dilation approach has two related problems. The first you clearly pointed out, tol ends up being coerced into multiples of the voxel size. The second is that this multiple ends up depending on orientation. For example an roi cented at (0, 0) with tol=1 will reject points at (1.001, 0), (0, 1.001) and (1.001, 1.001). The first two are > 1 from (0, 0) but the last is a distance > sqrt(2) from (0, 0).

I would argue that both the issues with the dilation method are fundamental to representing ROIs as masks. I believe the select method should only be as precise as it's inputs. The ROI input is already "discretized", for example if I want an ROI that follows the freesurfer pial surface, I need to "discretize" it into a mask in order to pass it to the select method. I would argue that once this is done, trying to create artificial precision is not very useful.

That being said, I've wanted to create a way of representing ROIs as closed 3D surfaces for some time, but when I bring it up people tend to run away screaming :).

@arokem

This comment has been minimized.

Member

arokem commented May 9, 2015

Yes - I think that enforcing a minimal tolerance so that the resulting ROI
covers the entire mask is reasonable. Another possibility is to use the
edges of the voxels as the frame of reference, rather than the centers. Let
me think about this a bit more, and integrate all these comments into
something coherent.

By the way - I am not running away screaming from the notion of
representing ROIs as closed surfaces. I think that we need that for
visualization purposes anyway: creating triangular meshes from ROIs via a
delaunay triangulation. There are some tricky issues to be tackled there,
to be sure, but it's eminently doable. Do you think we should start with
that first, before doing this here? I am not sure how it would solve all
the problems we have here. That is, you would still need to bound on
distance from the mesh somehow.

On Fri, May 8, 2015 at 12:24 PM, Bago Amirbekian notifications@github.com
wrote:

@arokem https://github.com/arokem I understand your concern and see
some of the advantages of your current definition. I believe both
definitions have some issues. For example if you think of voxels as
spheres, and set that diameter to be the edge length of one voxel, you'll
end up with a grid of non-overlapping spheres and you'll miss points that
lie in the space between these spheres. The result is that your ROI will
have internal gaps. To avoid gaps in a 3D ROI you'll need to set tol to be

= sqrt(sum((voxel_edges / 2)**2)).

The dilation approach has two related problems. The first you clearly
pointed out, tol ends up being coerced into multiples of the voxel size.
The second is that this multiple ends up depending on orientation. For
example an roi cented at (0, 0) with tol=1 will reject points at (1.001,
0), (0, 1.001) and (1.001, 1.001). The first two are > 1 from (0, 0) but
the last is a distance > sqrt(2) from (0, 0).

I would argue that both the issues with the dilation method are
fundamental to representing ROIs as masks. I believe the select method
should only be as precise as it's inputs. The ROI input is already
"discretized", for example if I want an ROI that follows the freesurfer
pial surface, I need to "discretize" it into a mask in order to pass it to
the select method. I would argue that once this is done, trying to create
artificial precision is not very useful.

That being said, I've wanted to create a way of representing ROIs as
closed 3D surfaces for some time, but when I bring it up people tend to run
away screaming :).


Reply to this email directly or view it on GitHub
#635 (comment).

@arokem arokem force-pushed the arokem:intersect-with-roi branch from e27aa27 to 35025bf May 20, 2015

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

OK - we've decided to take the middle road here: we stick to the tolerance-setting approach (little spheres around the centers of voxels), but we are enforcing a minimal reasonable tolerance that prevents users from using this with gaps in their ROI.

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

We still need to do:

  • Implement this as a generator
  • Use only the streamline endpoints for testing 'nearness'
  • Sort out how to use numpy better for the logical operations (select and reduce)
@MrBago

This comment has been minimized.

Contributor

MrBago commented May 20, 2015

@arokem I think you missed my point about the logical operations. My suggestion is that you merge all the include rois and exclude rois into two rois (agg_include and agg_exlude). Then you just need to write a function that deals with two arrays instead of two lists of arrays, that'll make the code easier to read and hopefully faster.

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

Hmm. We do aggregate them inside the function into to_include and
to_exclude arrays. Is that what you meant?

On Wed, May 20, 2015 at 1:46 PM, Bago Amirbekian notifications@github.com
wrote:

@arokem https://github.com/arokem I think you missed my point about the
logical operations. My suggestion is that you merge all the include rois
and exclude rois into two rois (agg_include and agg_exlude). Then you just
need to write a function that deals with two arrays instead of two lists of
arrays, that'll make the code easier to read and hopefully faster.


Reply to this email directly or view it on GitHub
#635 (comment).

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 20, 2015

@arokem

This comment has been minimized.

Member

arokem commented May 29, 2015

OK - I think that I have completed the things I was hoping to do in this PR. The last few commits implement the selection functionality proposed by @MrBago, returns a generator of streamlines from the selection function, and then also allows an option where only the endpoints of the streamline are considered as criteria for selection. Finally, that last commit speeds up things by another 30% or so, in the timing benchmarks I tested. Anything more to do here, before this can be merged?

@mdesco

This comment has been minimized.

Contributor

mdesco commented May 29, 2015

Nice work guys! This really looks awesome and of great need for Dipy.

A related question or request that could go in a separate PR but I was wondering if any of you were already thinking about it.

One powerful thing that there is in TrackVis right now and track_vis (command line) version is to select streamlines going through a ROI that connects "either_end" or "both_ends" of the streamlines. Right now, you guys have implemented the "any part" or "no part" of a streamline. The either or both ends are quite powerful when you want to select things ending in a region.

Could that be added as a flag to the current functions we have in this PR?
Also, you might be interested to have a look at the latest tck2connectome (https://github.com/MRtrix3/mrtrix3/wiki/tck2connectome) in Mrtrix 3, they have cool ways to deal with the end points radius, order, etc. This is also very crucial as you approach the cortex or subcortical regions.

As I said, this might be too much for this PR. I just thought I would bring it up now because it is related to the current PR. Otherwise, I think this PR is ready to be merged.

Cheers

@arokem

This comment has been minimized.

Member

arokem commented May 30, 2015

Hey @mdesco - thanks for the feedback!

I believe that we basically already have the either_end covered in the last few commits, and I just need to finish the implementation so that select also exposes this functionality. I will definitely do this here. It shouldn't be too hard to extend this to have the both_end criterion as well, and I can see why that would also be useful. I will do that here in this PR, so please don't merge yet (will change the title to WIP, just to be sure).

Also: @MrBago - have not tested this with generator input, so need to do that as well, before proceeding with this.

@arokem

This comment has been minimized.

Member

arokem commented May 30, 2015

TODO list:

  • Implement both either_end and both_end kwargs all the way from select down.
  • Test select with generator inputs.

@arokem arokem changed the title from Select streamlines based on logical operations on ROIs to WIP: Select streamlines based on logical operations on ROIs May 30, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jun 1, 2015

This is now substantially refactored to make it work with generator input and produce generator output. This required some separation of near_roi and select_from_roi, because creating the boolean value in near_roi required consuming the generator. I have instead implemented the inner cython loop on a streamline by streamline basis. As it turns out, this is actually faster than iterating of streamlines in cython. I've also managed to make this go a bit faster, by implementing distance as a c function.

I have implemented four different modes: "all", "any", "either_end" and "both_end". The selection function select_from_roi allows setting these modes, but the same mode should be used for all ROIs.

I've added some documentation examples showing how to use this.

@arokem arokem changed the title from WIP: Select streamlines based on logical operations on ROIs to Select streamlines based on logical operations on ROIs Jun 1, 2015

@arokem arokem force-pushed the arokem:intersect-with-roi branch from 8dd0be7 to abc5028 Jun 1, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2015

Any further comments here? Or is this ready to go?

@arokem arokem force-pushed the arokem:intersect-with-roi branch from 05c3106 to 06eceaf Jul 9, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jul 9, 2015

Re-rebased. Any further comments here?

arokem added some commits Aug 14, 2015

DOC: Improve rendering of docs.
In particular, undo changes proposed by @Garyfallidis. Turns out they
weren't needed.

@arokem arokem force-pushed the arokem:intersect-with-roi branch from 0065a9e to f6ee706 Aug 23, 2015

arokem added some commits Aug 23, 2015

A few more comments from @Garyfallidis.
Also - incorporate recent changes from upstream (including changes in
tools/make_examples.py).
@arokem

This comment has been minimized.

Member

arokem commented Aug 23, 2015

OK - hopefully we got it all this time. I think I've addressed that recent wave of comments (but let me know if you still think functions need to be moved around further), and I've added a couple of tests for reduce_rois on its own.

>>> mask1[0, 0, 0] = True
>>> mask2[1, 0, 0] = True
>>> selection = select_by_rois(streamlines, [mask1, mask2], [True, True],
... tol=1)

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 23, 2015

Member

Here the tol parameter is not aligned with streamlines.

[ 0., 1., 1.],
[ 0., 2., 2.]])]
>>> selection = select_by_rois(streamlines, [mask1, mask2], [True, False],
... tol=0.87)

This comment has been minimized.

@Garyfallidis
[ 0., 1., 1.],
[ 0., 2., 2.]])]
>>> selection = select_by_rois(streamlines, [mask1, mask2],
... [True, True],

This comment has been minimized.

@Garyfallidis
[ 1.9, 0. , 0. ]])]
>>> mask2[0, 2, 2] = True
>>> selection = select_by_rois(streamlines, [mask1, mask2],
... [True, True],

This comment has been minimized.

@Garyfallidis
"both_end" : both end points are within tol from ROI.
Returns
-------

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 23, 2015

Member

underline is off

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 23, 2015

A few more things @arokem

# Setting tolerance too low gets overridden:
selection = select_by_rois(streamlines, [mask1, mask2], [True, False],
tol=0.1)

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 23, 2015

Member

tol not aligned (pep8)

# Use different modes:
selection = select_by_rois(streamlines, [mask1, mask2, mask3],
[True, True, False],

This comment has been minimized.

@Garyfallidis
selection = select_by_rois(streamlines, [mask1, mask2, mask3],
[True, True, False],

This comment has been minimized.

@Garyfallidis
npt.assert_array_equal(list(selection), [streamlines[0]])
selection = select_by_rois(streamlines, [mask1, mask2, mask3],
[True, True, False],

This comment has been minimized.

@Garyfallidis
mask2[0, 2, 2] = True
selection = select_by_rois(streamlines, [mask1, mask2, mask3],
[True, True, False],

This comment has been minimized.

@Garyfallidis
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 23, 2015

Hey man. I think the editor you are currently using does not give you correct indications for pep8. I am not used on spending much time on correcting pep8 issues with your PRs. Did you change something recently on your editor settings? Anyway, please resolve any last styling issues.

arokem added some commits Aug 23, 2015

@arokem

This comment has been minimized.

Member

arokem commented Aug 23, 2015

OK - done.

On Sun, Aug 23, 2015 at 2:04 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hey man. I think the editor you are currently using does not give you
correct indications for pep8.


Reply to this email directly or view it on GitHub
#635 (comment).

mask[0, 0, 0] = True
mask[0, 2, 2] = True
assert_array_equal(near_roi(streamlines, mask, mode="both_end"),
np.array([False, True, False]))

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 23, 2015

Member

Nope you still have styling issues. Look here.

np.array([True, False, False]))
assert_array_equal(near_roi(streamlines, mask, tol=0.87, mode="both_end"),
np.array([False, False, False]))

This comment has been minimized.

@Garyfallidis

Garyfallidis Aug 23, 2015

Member

And here. Please check again your entire PR.

arokem added some commits Aug 23, 2015

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 24, 2015

:)

Garyfallidis added a commit that referenced this pull request Aug 24, 2015

Merge pull request #635 from arokem/intersect-with-roi
Select streamlines based on logical operations on ROIs

@Garyfallidis Garyfallidis merged commit 6fb01a6 into nipy:master Aug 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jchoude

This comment has been minimized.

Contributor

jchoude commented Aug 24, 2015

Just a +1 on not replacing target with the new functions.

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