Skip to content

Commit

Permalink
Match Octave/Numpy's minimum RWORK size for gesdd. Fixes #4016
Browse files Browse the repository at this point in the history
gesdd's RWORK size was recently changed to match the netlib header.
However, the minimum size calculated results in a segfault.  Both
Octave and Numpy use a different minimum size, and testing verified
that anything smaller than this leads to a segfault.

Numpy: https://github.com/numpy/numpy/blob/master/numpy/linalg/umath_linalg.c.src#L2922
Octave: http://hg.savannah.gnu.org/hgweb/octave/file/2f1729cae08f/liboctave/numeric/CmplxSVD.cc#l184
  • Loading branch information
kmsquire committed Aug 13, 2013
1 parent 3134f78 commit 9f60588
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion base/linalg/lapack.jl
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ for (geev, gesvd, gesdd, ggsvd, elty, relty) in
S = Array($relty, minmn)
cmplx = iseltype(A,Complex)
if cmplx
rwork = Array($relty, job == 'N' ? 5*minmn :
rwork = Array($relty, job == 'N' ? 7*minmn :
minmn*max(5*minmn+7, 2*max(m,n)+2*minmn+1))
end
iwork = Array(BlasInt, 8*minmn)
Expand Down

5 comments on commit 9f60588

@Keno
Copy link
Member

@Keno Keno commented on 9f60588 Aug 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kmsquire Thanks for tracking this down. I just committed patches to dlasd4 and slasd4 as well as part of #2430 - but you have rebuild openblas if you want these fixes. Hopefully with all of this will make our SVD bugfree.

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the Numpy and octave wrappers, I feel that our wrappers are considerably simpler and easier to hack.

@kmsquire
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Looking at 484a9f0, matching Numpy and Octave was kind of irrelevant, because we used to have exactly what they have. Either way, the 7*minnm needs to be there.

@kmsquire
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ViralBShah, hopefully so! I think you meant #2340. Cheers!

Please sign in to comment.