Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix for new name of scipy.linalg.blas.fblas #1150

Merged
merged 2 commits into from Jan 8, 2013

Conversation

Projects
None yet
3 participants
Member

delallea commented Jan 7, 2013

Also cleaned up duplicated / useless code at the same time.
This fixes gh-1144.

Fix for new name of scipy.linalg.blas.fblas
Also cleaned up duplicated / useless code at the same time.
This fixes gh-1144.

@mforbes mforbes commented on the diff Jan 7, 2013

theano/tensor/blas.py
@@ -150,16 +150,22 @@
try:
import scipy.linalg.blas
- _have_fblas = True
+ have_fblas = True
+ try:
@mforbes

mforbes Jan 7, 2013

It might be more explicit to check for the scipy version numbers with scipy.version.version. (I think the API changed in 0.10.0).

@nouiz

nouiz Jan 7, 2013

Owner

I prefer the not explicit version as this make it more robust in case of changes. But adding the version with the change in comment would be great. @delallea, can you add it?

@delallea

delallea Jan 7, 2013

Member

I can but we should make sure exactly which version it is. 0.10 seems surprising to me as I would have expected to see a bug report sooner.

@nouiz

nouiz Jan 7, 2013

Owner

I looked into the git history of scipy and it is included only in the trunk of scipy. So this will be for version > 0.11.0 as 0.11 is the last version and it is not changed. @mforbes, can you confirm the version of SciPy that you use?

@mforbes

mforbes Jan 8, 2013

I was using the latest development trunk. I think the comments in the code are now correct.

Owner

nouiz commented Jan 7, 2013

I added a comment to the scipy PR that broke this:

scipy/scipy#358

Member

delallea commented Jan 7, 2013

I pushed another commit that gives more details on when & where the Scipy change occurred.

@mforbes mforbes commented on the diff Jan 8, 2013

theano/tensor/blas.py
@@ -150,16 +150,23 @@
try:
import scipy.linalg.blas
- _have_fblas = True
+ have_fblas = True
+ try:
+ fblas = scipy.linalg.blas.fblas
+ except AttributeError:
+ # A change merged in Scipy development version on 2012-12-02 replaced
+ # `scipy.linalg.blas.fblas` with `scipy.linalg.blas`.
+ # See http://github.com/scipy/scipy/pull/358
+ fblas = scipy.linalg.blas
@mforbes

mforbes Jan 8, 2013

This is correct. Some more details:

  • The new code 'fblas = scipy.linalg.blas' will work as of version 0.10.0 when scipy.linalg.blas has all of the fblas routines exposed.
  • The old code will fail as of revision 812c886b448276617f3094f0693c5c54bc97d85f which changed scipy.linalg.blas.fblas to scipy.linalg.blas._fblas. This is a recent change as @nouiz pointed out and explains why this bug was only noticed recently. (I am using the latest version of scipy from github to deal with other BLAS issues.)
  • scipy.linalg.blas._fblas is completely removed in a89748ba10e689973d74a23be00b4566b7c03bcb.

The try form is probably fine and more robust. My only complaint was that it first tries a deprecated form of the API and the falls back to the supported form, but I understand that in this case, it is somewhat tricky to fall-back to the old code.

As far as I can tell, this PR now looks good and works for me.

nouiz added a commit that referenced this pull request Jan 8, 2013

Merge pull request #1150 from delallea/bugfix
Fix for new name of scipy.linalg.blas.fblas

@nouiz nouiz merged commit 55f6a18 into Theano:master Jan 8, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment