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

BF: Differences in spherical harmonic calculations wrt scipy 0.15 #612

Merged
merged 7 commits into from Jun 5, 2015

Conversation

Projects
None yet
7 participants
@arokem
Member

arokem commented Mar 20, 2015

Addresses #611 ?

@MrBago

This comment has been minimized.

Contributor

MrBago commented Mar 20, 2015

This isn't a precision issue, it doesn't match because you changed the sphere :). This test is meant to ensure that in the process of updating the CSD model, we didn't change the default values of lambda and such.

If you use a different sphere these values will change a little, which is normal, but the precomputed values were created using "symmetric362".

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 20, 2015

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2015

Well, to continue with the animal metaphors, the selection of sphere turns out to be a red herring (but it's my bad for mixing it in): the test passes on current master with python 2.7 regardless of selection of sphere in this test, and fails on python3.4, also regardless of the sphere (symmetric362 or default_sphere alike).

@MrBago

This comment has been minimized.

Contributor

MrBago commented Mar 20, 2015

It means the lambda calculation is (slightly) different between python2.7 and python3.4. Might not be a big deal, but I'd feel better if we tracked it down. Is this happening for all SH orders or only 16?

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2015

Happens both with 8 and 16

On Fri, Mar 20, 2015 at 1:04 PM, MrBago notifications@github.com wrote:

It means the lambda calculation is (slightly) different between python2.7
and python3.4. Might not be a big deal, but I'd feel better if we tracked
it down. Is this happening for all SH orders or only 16?


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

@MrBago

This comment has been minimized.

Contributor

MrBago commented Mar 20, 2015

K, will look into this when I have a chance. This PR is fine, but as a separate issue I'd like to know why lambdas are different with python versions.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 20, 2015

Fails on everything with commit f35702c (0.9.2). Could this be a numpy version problem?

https://travis-ci.org/MacPython/dipy-wheels

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2015

I have the same numpy version (1.9.1) on both 2.7 and 3.4

On Fri, Mar 20, 2015 at 1:54 PM, Matthew Brett notifications@github.com
wrote:

Fails on everything with commit f35702c
f35702c
(0.9.2). Could this be a numpy version problem?

https://travis-ci.org/MacPython/dipy-wheels


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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 20, 2015

Might be conda / MKL thing?

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2015

I'm on Anaconda. What is this Travis bot running?

On Fri, Mar 20, 2015 at 2:03 PM, Matthew Brett notifications@github.com
wrote:

Might be conda / MKL thing?


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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 20, 2015

Python.org Python with standard wheels.

@matthew-brett

This comment has been minimized.

@arokem

This comment has been minimized.

Member

arokem commented Mar 21, 2015

Probably not. I don't see this one on my laptop/python3

On Fri, Mar 20, 2015 at 2:59 PM, Matthew Brett notifications@github.com
wrote:

Are these problems related ?
http://nipy.bic.berkeley.edu/builders/dipy-py3.3-pip/builds/168/steps/shell_4/logs/stdio


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

@arokem

This comment has been minimized.

Member

arokem commented Mar 21, 2015

Also, any idea why this build worked?

http://nipy.bic.berkeley.edu/builders/dipy-py3.3-pip/builds/167

IIUC, they're pip installing the same release. What changed on this machine
between March 19th and March 20th?

On Fri, Mar 20, 2015 at 5:31 PM, Ariel Rokem arokem@gmail.com wrote:

Probably not. I don't see this one on my laptop/python3

On Fri, Mar 20, 2015 at 2:59 PM, Matthew Brett notifications@github.com
wrote:

Are these problems related ?
http://nipy.bic.berkeley.edu/builders/dipy-py3.3-pip/builds/168/steps/shell_4/logs/stdio


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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 21, 2015

Sorry to get off topic. The other failure went away with a simple rerun - looks like some randomness in the tests - I'll open another issue.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 21, 2015

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

Back to this topic: do we know what changed on our 3.3 worker between March 19th and 20th? It seems to explain this issue.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 6, 2015

See #613 - the test failure was due to memory access violations, unrelated to this issue, now fixed in master.

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

Oh yes, you're right. So we still don't understand this issue, huh? Is Conda/MKL still a plausible culprit?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 6, 2015

I guess it could be Conda +/- MKL - I'm afraid I don't have conda / MKL installed anywhere.

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

Yeah - I've heard it's really tough to install ;-)

So - should we go ahead with this PR? We do want to support conda/mkl, as
well as non-conda/mkl, and it seems that this is how accurate we can get in
matching the two. Is that a fair assessment?

On Mon, Apr 6, 2015 at 2:32 PM, Matthew Brett notifications@github.com
wrote:

I guess it could be Conda +/- MKL - I'm afraid I don't have conda / MKL
installed anywhere.


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

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

Oh wait, I might not have MKL. Anyone know how to check that? Or is
Anaconda always MKL-powered?

On Mon, Apr 6, 2015 at 2:37 PM, Ariel Rokem arokem@gmail.com wrote:

Yeah - I've heard it's really tough to install ;-)

So - should we go ahead with this PR? We do want to support conda/mkl, as
well as non-conda/mkl, and it seems that this is how accurate we can get in
matching the two. Is that a fair assessment?

On Mon, Apr 6, 2015 at 2:32 PM, Matthew Brett notifications@github.com
wrote:

I guess it could be Conda +/- MKL - I'm afraid I don't have conda / MKL
installed anywhere.


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

@ghost

This comment has been minimized.

ghost commented Apr 6, 2015

I think the academic version should come with MKL, but the free version
available to the public does not have it.

Bago

On Mon, Apr 6, 2015 at 2:39 PM, Ariel Rokem notifications@github.com
wrote:

Oh wait, I might not have MKL. Anyone know how to check that? Or is
Anaconda always MKL-powered?

On Mon, Apr 6, 2015 at 2:37 PM, Ariel Rokem arokem@gmail.com wrote:

Yeah - I've heard it's really tough to install ;-)

So - should we go ahead with this PR? We do want to support conda/mkl, as
well as non-conda/mkl, and it seems that this is how accurate we can get
in
matching the two. Is that a fair assessment?

On Mon, Apr 6, 2015 at 2:32 PM, Matthew Brett notifications@github.com
wrote:

I guess it could be Conda +/- MKL - I'm afraid I don't have conda / MKL
installed anywhere.


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


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

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

OK - I definitely did not have MKL before (but now I do!), so the
difference is not about MKL.

On Mon, Apr 6, 2015 at 2:42 PM, DaveOlg notifications@github.com wrote:

I think the academic version should come with MKL, but the free version
available to the public does not have it.

Bago

On Mon, Apr 6, 2015 at 2:39 PM, Ariel Rokem notifications@github.com
wrote:

Oh wait, I might not have MKL. Anyone know how to check that? Or is
Anaconda always MKL-powered?

On Mon, Apr 6, 2015 at 2:37 PM, Ariel Rokem arokem@gmail.com wrote:

Yeah - I've heard it's really tough to install ;-)

So - should we go ahead with this PR? We do want to support conda/mkl,
as
well as non-conda/mkl, and it seems that this is how accurate we can
get
in
matching the two. Is that a fair assessment?

On Mon, Apr 6, 2015 at 2:32 PM, Matthew Brett <
notifications@github.com>
wrote:

I guess it could be Conda +/- MKL - I'm afraid I don't have conda /
MKL
installed anywhere.


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


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


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

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

Whether or not I have MKL installed, this does fail.

On a side note - MKL is fast!

On Mon, Apr 6, 2015 at 2:48 PM, Ariel Rokem arokem@gmail.com wrote:

OK - I definitely did not have MKL before (but now I do!), so the
difference is not about MKL.

On Mon, Apr 6, 2015 at 2:42 PM, DaveOlg notifications@github.com wrote:

I think the academic version should come with MKL, but the free version
available to the public does not have it.

Bago

On Mon, Apr 6, 2015 at 2:39 PM, Ariel Rokem notifications@github.com
wrote:

Oh wait, I might not have MKL. Anyone know how to check that? Or is
Anaconda always MKL-powered?

On Mon, Apr 6, 2015 at 2:37 PM, Ariel Rokem arokem@gmail.com wrote:

Yeah - I've heard it's really tough to install ;-)

So - should we go ahead with this PR? We do want to support
conda/mkl, as
well as non-conda/mkl, and it seems that this is how accurate we can
get
in
matching the two. Is that a fair assessment?

On Mon, Apr 6, 2015 at 2:32 PM, Matthew Brett <
notifications@github.com>
wrote:

I guess it could be Conda +/- MKL - I'm afraid I don't have conda /
MKL
installed anywhere.


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


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


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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 6, 2015

Ariel - did you try using pip installed numpy etc ?

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

No - pip install scipy doesn't work for me. I'm getting some compilation
error.

On Mon, Apr 6, 2015 at 3:01 PM, Matthew Brett notifications@github.com
wrote:

Ariel - did you try using pip installed numpy etc ?


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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 6, 2015

Yes, sorry, you'd need to install a standard Python.org or macports or homebrew Python, then pip install numpy scipy cython should be a few seconds of download / install. It won't work with conda because conda has an old (10.5) OSX distutils version string, so won't pick up the wheels.

@arokem

This comment has been minimized.

Member

arokem commented Apr 7, 2015

OK - that wasn't too bad actually. I might even see myself recommending
this route for installation for beginners, once we sort out the VTK
situation (e.g. find a way to pip install that). And also, I found it
potentially a bit confusing that the installation only creates the
'python3.4' name in bash (in that weird mac-y /Library/Frameworks
location), but not 'python'. Any particular reason for that? I figure I
could hack an alias or a symlink, but that's not easy to explain to a
beginner, so I would love to have a more straightforward route.

At any rate - I see the same test-failure with the python.org python and
pip-installed everything. So, this is apparently not about MKL/Anaconda.
Should I try downgrading to OS 10.9? ;-)

On Mon, Apr 6, 2015 at 3:35 PM, Matthew Brett notifications@github.com
wrote:

Yes, sorry, you'd need to install a standard Python.org or macports or
homebrew Python, then pip install numpy scipy cython should be a few
seconds of download / install. It won't work with conda because conda has
an OSX distutils version string, so won't pick up the wheels.


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

@arokem

This comment has been minimized.

Member

arokem commented May 22, 2015

Hmph. I am now getting this on python 2.7 (but not 3.4):


======================================================================

ERROR: Failure: AttributeError ('numpy.ufunc' object has no attribute
'__module__')

----------------------------------------------------------------------

Traceback (most recent call last):

  File
"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/loader.py",
line 420, in loadTestsFromName

    addr.filename, addr.module)

  File
"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/importer.py",
line 47, in importFromPath

    return self.importFromDir(dir_path, fqname)

  File
"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/importer.py",
line 94, in importFromDir

    mod = load_module(part_fqname, fh, filename, desc)

  File "/Users/arokem/source/dipy/dipy/reconst/tests/test_csdeconv.py",
line 13, in <module>

    from dipy.reconst.csdeconv import (ConstrainedSphericalDeconvModel,

  File "/Users/arokem/source/dipy/dipy/reconst/csdeconv.py", line 19, in
<module>

    from dipy.reconst.shm import (sph_harm_ind_list, real_sph_harm,

  File "/Users/arokem/source/dipy/dipy/reconst/shm.py", line 198, in
<module>

    def spherical_harmonics(m, n, theta, phi, dtype=complex):

  File "/Users/arokem/anaconda/envs/py2/lib/python2.7/functools.py", line
33, in update_wrapper

    setattr(wrapper, attr, getattr(wrapped, attr))

AttributeError: 'numpy.ufunc' object has no attribute '__module__'


----------------------------------------------------------------------

Ran 1 test in 0.001s

I think that has to do with @MrBago's recent suggestion, because when I
back off from that, everything seems fine.

On Wed, May 20, 2015 at 11:02 AM, argriffing notifications@github.com
wrote:

After having the added the most recent commit to this PR, the sph_harm
import should be added back and the sphere should be changed back to
symmetric362, right?


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

@arokem

This comment has been minimized.

Member

arokem commented May 22, 2015

Concerning the sphere: I think that we want to move ahead with the default
sphere.

On Thu, May 21, 2015 at 5:30 PM, Ariel Rokem arokem@gmail.com wrote:

Hmph. I am now getting this on python 2.7 (but not 3.4):


======================================================================

ERROR: Failure: AttributeError ('numpy.ufunc' object has no attribute
'__module__')

----------------------------------------------------------------------

Traceback (most recent call last):

  File
"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/loader.py",
line 420, in loadTestsFromName

    addr.filename, addr.module)

  File
"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/importer.py",
line 47, in importFromPath

    return self.importFromDir(dir_path, fqname)

  File
"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/importer.py",
line 94, in importFromDir

    mod = load_module(part_fqname, fh, filename, desc)

  File "/Users/arokem/source/dipy/dipy/reconst/tests/test_csdeconv.py",
line 13, in <module>

    from dipy.reconst.csdeconv import (ConstrainedSphericalDeconvModel,

  File "/Users/arokem/source/dipy/dipy/reconst/csdeconv.py", line 19, in
<module>

    from dipy.reconst.shm import (sph_harm_ind_list, real_sph_harm,

  File "/Users/arokem/source/dipy/dipy/reconst/shm.py", line 198, in
<module>

    def spherical_harmonics(m, n, theta, phi, dtype=complex):

  File "/Users/arokem/anaconda/envs/py2/lib/python2.7/functools.py", line
33, in update_wrapper

    setattr(wrapper, attr, getattr(wrapped, attr))

AttributeError: 'numpy.ufunc' object has no attribute '__module__'


----------------------------------------------------------------------

Ran 1 test in 0.001s

I think that has to do with @MrBago's recent suggestion, because when I
back off from that, everything seems fine.

On Wed, May 20, 2015 at 11:02 AM, argriffing notifications@github.com
wrote:

After having the added the most recent commit to this PR, the sph_harm
import should be added back and the sphere should be changed back to
symmetric362, right?


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

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 22, 2015

Sorry I didn't test it before I pushed it, apparently @wraps chokes on
ufuncs,

@wraps(numpy.add)
def foo():
    pass

Feel free to take out the @wraps line, or my whole change.

On Thu, May 21, 2015 at 5:31 PM, Ariel Rokem notifications@github.com
wrote:

Hmph. I am now getting this on python 2.7 (but not 3.4):


======================================================================

ERROR: Failure: AttributeError ('numpy.ufunc' object has no attribute
'__module__')

----------------------------------------------------------------------

Traceback (most recent call last):

File

"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/loader.py",
line 420, in loadTestsFromName

addr.filename, addr.module)

File

"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/importer.py",
line 47, in importFromPath

return self.importFromDir(dir_path, fqname)

File

"/Users/arokem/anaconda/envs/py2/lib/python2.7/site-packages/nose/importer.py",
line 94, in importFromDir

mod = load_module(part_fqname, fh, filename, desc)

File "/Users/arokem/source/dipy/dipy/reconst/tests/test_csdeconv.py",
line 13, in <module>

from dipy.reconst.csdeconv import (ConstrainedSphericalDeconvModel,

File "/Users/arokem/source/dipy/dipy/reconst/csdeconv.py", line 19, in
<module>

from dipy.reconst.shm import (sph_harm_ind_list, real_sph_harm,

File "/Users/arokem/source/dipy/dipy/reconst/shm.py", line 198, in
<module>

def spherical_harmonics(m, n, theta, phi, dtype=complex):

File "/Users/arokem/anaconda/envs/py2/lib/python2.7/functools.py", line
33, in update_wrapper

setattr(wrapper, attr, getattr(wrapped, attr))

AttributeError: 'numpy.ufunc' object has no attribute '__module__'


----------------------------------------------------------------------

Ran 1 test in 0.001s

I think that has to do with @MrBago's recent suggestion, because when I
back off from that, everything seems fine.

On Wed, May 20, 2015 at 11:02 AM, argriffing notifications@github.com
wrote:

After having the added the most recent commit to this PR, the sph_harm
import should be added back and the sphere should be changed back to
symmetric362, right?


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


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

@stefanv

This comment has been minimized.

Contributor

stefanv commented May 22, 2015

@MrBago Can you explain? This works, e.g.:

import numpy as np
from functools import wraps

def my_decorator(f):
    @wraps(f)
    def wrapper(*args, **kwds):
        print('Calling decorated function')
        return f(*args, **kwds)
    return wrapper

f = my_decorator(np.add)
print(f(1, 2))

@stefanv

This comment has been minimized.

Contributor

stefanv commented May 22, 2015

I have numpy 1.10 with Python 3.4.

@arokem

This comment has been minimized.

Member

arokem commented May 22, 2015

I think it's a python 3 <=> 2 thing. That snippet failed on 2.7.9, and
passed on 3.4.3, on my machine, same numpy (1.9.2) on both.

On Thu, May 21, 2015 at 9:15 PM, Stefan van der Walt <
notifications@github.com> wrote:

I have numpy 1.10 with Python 3.4.


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

@arokem arokem force-pushed the arokem:test-lamda-csdeconv branch from b1403c8 to 8333a9d May 22, 2015

@arokem

This comment has been minimized.

Member

arokem commented May 22, 2015

I just wanted to point out that this is potentially the right way to deal with this issue, but am still waiting to hear back on #652.

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 22, 2015

The problem with #652 is that we still have functions like
shm.real_sh_basis that will exhibit the bad behaviour if our users are
unlucky enough to use them with the exact wrong set of arguments. Ideally
we would fix this at the sph_harm level.

On Fri, May 22, 2015 at 11:48 AM, Ariel Rokem notifications@github.com
wrote:

I just wanted to point out that this is potentially the right way to deal
with this issue, but am still waiting to hear back on #652
#652.


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

@arokem

This comment has been minimized.

Member

arokem commented May 22, 2015

Oh yeah - I am not proposing to merge #652 instead of this one.

Just pointing out that the discussion there about the right way to handle this issue is still ongoing. So far, it seems like the way implemented here is the right way to handle this issue for now.

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 22, 2015

The only reason I used @Wrap was so I didn't have to write a docstring, we
can just drop @Wrap and make the docstring better and it should work.

Bago

On Fri, May 22, 2015 at 2:59 PM, Ariel Rokem notifications@github.com
wrote:

Oh yeah - I am not proposing to merge #652
#652 instead of this one.

Just pointing out that the discussion there about the right way to handle
this issue is still ongoing. So far, it seems like the way implemented here
is the right way to handle this issue for now.


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

@arokem

This comment has been minimized.

Member

arokem commented May 22, 2015

How about this then? Docstring slightly improved to indicate that we use scipy in cases where version >= 0.15

@MrBago

This comment has been minimized.

Contributor

MrBago commented May 22, 2015

I seem to have done something odd to my github account...

but +1 for the docstring!

On Fri, May 22, 2015 at 3:54 PM, Ariel Rokem notifications@github.com
wrote:

How about this then? Docstring slightly improved to indicate that we use
scipy in cases where version >= 0.15


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

@arokem

This comment has been minimized.

Member

arokem commented May 29, 2015

No further discussions on whether this is really the ultimate best-practice for handling this. It seems like this is at the very least an acceptable solution. Any reason not to merge this fix?

@argriffing

This comment has been minimized.

argriffing commented May 29, 2015

For what it's worth, I just merged the scipy-level workaround in scipy/scipy#4896, and the underlying numpy bug numpy/numpy#5895 is still open.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented May 30, 2015

Hey @arokem I am lost a bit in the discussion of this PR. Is this ready to be merged now? Do we continue using the scipy functions after scipy 0.15 or stick to using the Dipy version?

@arokem

This comment has been minimized.

Member

arokem commented May 31, 2015

I think that it is ready to be merged.

We are using scipy from 0.15 onwards, but at least for 0.15 itself, we are taking into account that we need to specify the dtype kwarg. We might be able to do away with that workaround in future versions, and eventually also drop our implementation altogether.

@arokem arokem force-pushed the arokem:test-lamda-csdeconv branch from a9592e4 to a95c6a8 Jun 5, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jun 5, 2015

Are there any remaining objections here? Just rebased this.

stefanv added a commit that referenced this pull request Jun 5, 2015

Merge pull request #612 from arokem/test-lamda-csdeconv
BF: Differences in spherical harmonic calculations wrt scipy 0.15

@stefanv stefanv merged commit a6e3c29 into nipy:master Jun 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment