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

Line-based version of streamline_mapping #1150

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@nilgoyette
Contributor

nilgoyette commented Nov 17, 2016

I refactored the already existing line-based functions (_streamlines_in_mask(), _streamline_in_mask()) to re-use Bresenham's algorithm. This made it possible to add a line-based version of streamline_mapping() without too much trouble.

I'm not totally satisfied with the code because

  • there's a little code copy
  • _streamline_info is used with 2 differents goals. (useless parameters and return)
  • It's somewhat slow. I don't see how I could be faster than streamline_mapping though because I do what it's doing + more stuff + with bigger data because we found more positions and more index.

I added a toy test AND tested with our reporting program which shows intersecting fibers. We had a lot of missing streamlines using streamline_mapping() on compressed fibers. The libe-based version fixes our problem perfectly; I obtain the same results as when I test all streamlines.

nilgoyette added some commits Nov 17, 2016

Add streamline_mapping_line_based
Refactor the inner function used by target_line_based() to re-use the
line_based code without making it slower. With this change, adding more
functions (like counting the number of streamlines passing in all
voxels) should be easier.
@coveralls

This comment has been minimized.

coveralls commented Nov 17, 2016

Coverage Status

Coverage increased (+0.008%) to 88.012% when pulling 87795ad on nilgoyyou:streamline_mapping_lb into eec397d on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 17, 2016

Current coverage is 85.47% (diff: 100%)

Merging #1150 into master will increase coverage by <.01%

@@             master      #1150   diff @@
==========================================
  Files           214        214          
  Lines         24917      24934    +17   
  Methods           0          0          
  Messages          0          0          
  Branches       2526       2527     +1   
==========================================
+ Hits          21296      21313    +17   
  Misses         2989       2989          
  Partials        632        632          

Powered by Codecov. Last update e87b327...38b11cf

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 18, 2016

/dipy/tracking/vox2track.o -msse2 -mfpmath=sse -fopenmp
    dipy/tracking/vox2track.c:240:18: fatal error: vector: No such file or directory
    compilation terminated.
    error: command 'gcc' failed with exit status 1

Is there a problem with C++ compilation on your server? I specifically asked in setup.py for this file to be compiled using C++. Is it simply a problem or renaming the file from .c to .cpp?

@arokem

This comment has been minimized.

Member

arokem commented Nov 18, 2016

Just to be clear, you are referring to this error on Travis?

https://travis-ci.org/nipy/dipy/jobs/176803157

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 18, 2016

Yes.

@arokem

This comment has been minimized.

Member

arokem commented Nov 18, 2016

I am mystified. Seems to only be a problem on the sdist machine. I kicked that one back into action, in case this is a glitch.

@arokem

This comment has been minimized.

Member

arokem commented Nov 18, 2016

No joy. I am not sure what's going on.

Merge branch 'master' of https://github.com/nipy/dipy into streamline…
…_mapping_lb

Conflicts:
	dipy/tracking/vox2track.pyx
@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 21, 2016

I don't know how to fix and I can't reproduce the error, so I simply merged the fmin fix to see if it would change anything. It probably won't.

@coveralls

This comment has been minimized.

coveralls commented Nov 21, 2016

Coverage Status

Coverage increased (+0.008%) to 88.012% when pulling 38b11cf on nilgoyyou:streamline_mapping_lb into e87b327 on nipy:master.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 17, 2017

@nilgoyyou and @jchoude I need some feedback with this PR. Are we on it? Or all is lost?

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Feb 17, 2017

Oh, sorry, I forgot about this PR. I'll pull master and test it again.
It might give the same error which I never understood though. I'm not sure what was going on.

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Feb 17, 2017

Ok, no, I don't understand.
Here's the output when compiled on your server:

building 'dipy.tracking.vox2track' extension
gcc -pthread -fno-strict-aliasing -g -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -Isrc -I/home/travis/build/nipy/dipy/venv/lib/python2.7/site-packages/numpy/core/include -Ibuild -I/opt/python/2.7.9/include/python2.7 -c dipy/tracking/vox2track.c -o build/temp.linux-x86_64-2.7/dipy/tracking/vox2track.o -msse2 -mfpmath=sse -fopenmp
dipy/tracking/vox2track.c:240:18: fatal error: vector: No such file or directory

And on my VM

cythoning dipy/tracking/vox2track.pyx to dipy/tracking/vox2track.cpp
building 'dipy.tracking.vox2track' extension
x86_64-linux-gnu-gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fPIC -Isrc -I/usr/local/lib/python2.7/dist-packages/numpy/core/include -Ibuild -I/usr/include/python2.7 -c dipy/tracking/vox2track.cpp -o build/temp.linux-x86_64-2.7/dipy/tracking/vox2track.o -msse2 -mfpmath=sse -fopenmp
c++ -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -D_FORTIFY_SOURCE=2 -g -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/dipy/tracking/vox2track.o -o build/lib.linux-x86_64-2.7/dipy/tracking/vox2track.so -fopenmp

Notice the first pass of gcc, then c++ All the other builds look like that, except sdist.
I can't reproduce this error with python setup.py sdist or python setup_egg.py sdist. install also work. Everything I test finishes without error. I could at least try to fix it myself if I was able to reproduce it.

Someone have an idea?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 1, 2017

I remember that I had some problem with Cython/C++ when I implemented the clustering framework :( .

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 1, 2017

What version of Cython do you have installed on your WM? Mine is Cython 0.23.01 and you PR is working correctly on my machine. :/

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Mar 1, 2017

I tested with version 0.24 and 0.25.2; they both work well.

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Mar 31, 2017

Since I don't know how to fix this and nobody knows, I'll try to remove the C++ requirement and use only normal cython. I can probably replace the std::vector with a reusable ndarray. I'll try that in the next weeks.

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Feb 9, 2018

I'll close this PR for now because it took too long and I have no immediate plans to do it. I'll probably clean it and re-open it someday.

@nilgoyette nilgoyette closed this Feb 9, 2018

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