search

SciRuby / nmatrix

Dense and sparse linear algebra library for Ruby via SciRuby
Other
469 stars 133 forks source link

Rename clapack_laswp #388

Open wlevine opened 9 years ago

wlevine commented 9 years ago

We have a function called clapack_laswp, which permutes columns, but as far as I can tell, there is no equivalent function called clapack_?laswp in ATLAS, or anywhere else in the world (if you google clapack_laswp, you just end up with results for nmatrix). John, do you remember where this code came from? There is a standard laswp function in LAPACK, but it exchanges rows instead of columns. So at the least, I think we should rename this to prevent confusion.

Furthermore, it seems like when calling clapack_laswp the last element of the pivot vector is ignored, although I am not really sure how this is supposed to work.

Also, we have #laswp! method for NMatrix, which behaves differently from the clapack_laswp and friends, due to #346. I think the default "intuitive" behavior is good for a method #permute_columns, but not for a method name #laswp, since all the other functions called *laswp behave differently. So I guess I think that #permute_columns and #laswp should be different methods with different behavior and not aliased to each other.

translunar commented 9 years ago

The only reason ours does columns is our storage format. It's a clone of the algorithm, and it was called by some other ATLAS functions (which I may or may not have had time to implement).

wlevine commented 9 years ago

Yeah, but every other function we've adapted to have the expected behavior when operating on row-major matrices. That's why I think we should rename this one.

translunar commented 9 years ago

If it's not used, I'd say we should just remove it.

wlevine commented 9 years ago

It's used internally in our C code. I don't know if anyone uses the ruby interface, maybe @agisga, since he submitted #346

translunar commented 9 years ago

Okay. I say go ahead and rename it.

agisga commented 9 years ago

I don't know if anyone uses the ruby interface, maybe @agisga, since he submitted #346

I have used #permute_columns! in the unmerged pull request #336 only. That's when the issue with the unintuitive behaviour of the method came up. I still want to improve some things in said pull request, and I can adapt it to any new behaviour of #permute_columns at the same time.