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

TST - new DTI test to help develop min_signal handling #492

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@MrBago
Contributor

MrBago commented Dec 8, 2014

No description provided.

@MrBago MrBago force-pushed the MrBago:dti_min_signal_test branch from d6274f5 to fab795f Dec 8, 2014

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2014

Maybe this helps?
MrBago#12

@MrBago

This comment has been minimized.

Contributor

MrBago commented Dec 10, 2014

@arokem, take a look at the last commit, I think this might get us 60% of the way there.

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2014

Hang on - I am working on it. Give me half an hour.

On Wed, Dec 10, 2014 at 2:42 PM, MrBago notifications@github.com wrote:

@arokem https://github.com/arokem, take a look at the last commit, I
think this might get us 60% of the way there.


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

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 11, 2014

Hi guys,

I hope you are on top of the situation here. I started receiving e-mails from people saying that the Tensor is broken in master. Here is a related error just reported by Paolo Avesani.

ten_fit = ten_model.fit(data, mask)

File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 677, in fit
_self.args, *_self.kwargs)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1113, in wls_fit_tensor
min_signal, min_diffusivity)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1124, in _wls_iter
raise ValueError("The data in this voxel contains only zeros")
ValueError: The data in this voxel contains only zeros

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2014

We're working on it, but there are some inconsistencies that still need
discussion. If there is an urgency to reproduce previous (imperfect!)
behavior, you can consider merging #496

On Thursday, December 11, 2014, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi guys,

I hope you are on top of the situation here. I started receiving e-mails
from people saying that the Tensor is broken in master. Here is a related
error just reported by Paolo Avesani.

ten_fit = ten_model.fit(data, mask)

File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 677, in fit
_self.args, *_self.kwargs)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1113, in wls_fit_tensor
min_signal, min_diffusivity)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1124, in _wls_iter
raise ValueError("The data in this voxel contains only zeros")
ValueError: The data in this voxel contains only zeros


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

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2014

I wish people would use the mailing list and issues, instead of mailing
you. I suggest that you point out these channels to people emailing you.
Would also serve to reduce the direct pressure on you

On Thursday, December 11, 2014, Ariel Rokem arokem@gmail.com wrote:

We're working on it, but there are some inconsistencies that still need
discussion. If there is an urgency to reproduce previous (imperfect!)
behavior, you can consider merging #496

On Thursday, December 11, 2014, Eleftherios Garyfallidis <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Hi guys,

I hope you are on top of the situation here. I started receiving e-mails
from people saying that the Tensor is broken in master. Here is a related
error just reported by Paolo Avesani.

ten_fit = ten_model.fit(data, mask)

File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 677, in fit
_self.args, *_self.kwargs)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1113, in wls_fit_tensor
min_signal, min_diffusivity)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1124, in _wls_iter
raise ValueError("The data in this voxel contains only zeros")
ValueError: The data in this voxel contains only zeros


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

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 11, 2014

Yeah, I do that. Sometimes, I wonder if having a dipy-user list would help. Or promote people to write in nipy-user. Not sure what is the best way. But let's not worry about that right now.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Dec 11, 2014

Is that running the tests or on a random dataset ,which could have zero
only voxels, or if the data is already normalized with respect to the b0
could lead to strange results.
Le 2014-12-11 11:03, Eleftherios Garyfallidis a écrit :

Hi guys,

I hope you are on top of the situation here. I started receiving
e-mails from people saying that the Tensor is broken in master. Here
is a related error just reported by Paolo Avesani.

|ten_fit = ten_model.fit(data, mask)
|

File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 677, in fit
/self.args, */self.kwargs)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1113, in wls_fit_tensor
min_signal, min_diffusivity)
File "/Users/paolo/Software/miniconda/src/dipy/dipy/reconst/dti.py",
line 1124, in _wls_iter
raise ValueError("The data in this voxel contains only zeros")
ValueError: The data in this voxel contains only zeros


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

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2014

Does this test-cases work for you, @samuelstjean ?

arokem@415234d

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Dec 11, 2014

Just ran it on a freshly updates master, test works fine ,adding your
new test alos works fine.

Weirdly enough, I get a SSE2 file not found error though, probably has
something to do with Omar PR.
Le 2014-12-11 14:16, Ariel Rokem a écrit :

Does this test-cases work for you, @samuelstjean
https://github.com/samuelstjean ?

arokem/dipy@415234d
arokem@415234d


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

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 11, 2014

Hi Sam, can you share the error message you got, please?

On Thu, Dec 11, 2014 at 3:15 PM, Samuel St-Jean notifications@github.com
wrote:

Just ran it on a freshly updates master, test works fine ,adding your
new test alos works fine.

Weirdly enough, I get a SSE2 file not found error though, probably has
something to do with Omar PR.
Le 2014-12-11 14:16, Ariel Rokem a écrit :

Does this test-cases work for you, @samuelstjean
https://github.com/samuelstjean ?

arokem/dipy@415234d
<
arokem@415234d


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


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

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@arokem arokem referenced this pull request Dec 11, 2014

Merged

DTI `min_signal` #498

@arokem

This comment has been minimized.

Member

arokem commented Dec 11, 2014

Closed through #498

@arokem arokem closed this Dec 11, 2014

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Dec 11, 2014

I'm on debian wheezy, approximately 4 years old laptop, so it probably
has the SSE2 extension. Anyway, I get

samuel ~/git/dipy $ [dipy (upstream_master)] python setup.py build_ext -i
running build_ext
C compiler: gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2
-Wall -Wstrict-prototypes -fPIC

compile options: '-I/usr/include/python2.7 -c'
extra options: '/arch:SSE2'
gcc: test.c
gcc: error: /arch:SSE2: Aucun fichier ou dossier de ce type -->this
means file or folder not found, why is that?

gcc: error: /arch:SSE2: Aucun fichier ou dossier de ce type
Flags ['/arch:SSE2'] omitted because of compile or link error
C compiler: gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2
-Wall -Wstrict-prototypes -fPIC

compile options: '-I/usr/include/python2.7 -c'
extra options: '-msse2 -mfpmath=sse'
gcc: test.c
gcc -pthread test.o -o a.out
C compiler: gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2
-Wall -Wstrict-prototypes -fPIC

compile options: '-I/usr/include/python2.7 -c'
extra options: '-fopenmp'
gcc: test.c
gcc -pthread test.o -o a.out -fopenmp

Le 2014-12-11 16:20, Omar Ocegueda a écrit :

Hi Sam, can you share the error message you got, please?

On Thu, Dec 11, 2014 at 3:15 PM, Samuel St-Jean
notifications@github.com
wrote:

Just ran it on a freshly updates master, test works fine ,adding your
new test alos works fine.

Weirdly enough, I get a SSE2 file not found error though, probably has
something to do with Omar PR.
Le 2014-12-11 14:16, Ariel Rokem a écrit :

Does this test-cases work for you, @samuelstjean
https://github.com/samuelstjean ?

arokem/dipy@415234d
<

arokem@415234d


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


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

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar


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

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 11, 2014

Thank you Sam!, =)
that's correct, the flag "/arch:SSE2" is a visual C flag, that's why it was omited. The flags "-msse2 -mfpmath=sse" is the gcc flag that was used. I need to see if it is possible to hide those errors (from the optional flag Checker) to avoid scaring the user =)

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Dec 12, 2014

Ok, so the flag is set later on and works, why the scary error message then? That's weird indeed :/

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Dec 12, 2014

That's how the Checker works, it compiles a simple C-code string. If it raises an exception (producing the error message), the flag is not set for the compilation of the real cython extensions (and the corresponding DEF in the config.py and config.pxi is set to False). If the code did not raise an exception (thus, you don't see any error message) then the flag is set for the real cython extensions (and the corresponding DEF is set to True). You can see the compile error check here:
https://github.com/nipy/dipy/blob/master/setup_helpers.py#L132
and here for catching the link error:
https://github.com/nipy/dipy/blob/master/setup_helpers.py#L137

The flag "-msse2 -mfpmath=sse" is set later on and works. But the flag "/arch:SSE2" generated the error (which you reported) and was not set. The two flags are set to be checked here:
https://github.com/nipy/dipy/blob/master/setup.py#L133

You can verify with:

import dipy.config as cfg
cfg.USING_GCC_SSE2
True
cfg.cfg.USING_VC_SSE2
False

I hope it helps =)

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