scijs / ndarray-blas-level2

BLAS Level 2 operations for ndarrays
MIT License
9 stars 2 forks source link

Removed getters and setters from GEMV #12

Closed tab58 closed 8 years ago

tab58 commented 8 years ago

GEMV would fail for column vectors that were n x 1 matrices because get/set arguments would be wrong. Replacing with indices solves that problem.

tab58 commented 8 years ago

Any reason this shouldn't be merged in, @rreusser?

rreusser commented 8 years ago

Two concerns here:

  1. It breaks ndarrays of type generic that use get/set notation
  2. I think passing a 2D matrix with a singleton dimension to a matrix-vector product function should be expected to fail. I added an ndarray-squeeze module to address this. In other words, I think a REPL sort of environment could be expected to magically sweep this under the rug so that the user wouldn't need to care, but I think it's best if a low-level function avoids handling that.

That said, your version is somewhat faster (hard to quote a single number; depends on the dimensions. 30% faster for large dimensions, 500% faster for small dimensions?) and I rather prefer it. Perhaps the right answer is:

Of course they would all have the same argument signature so that you could require('ndarray-blas-level1/gemv.optimized') if you wanted to bypass the check for generic. Does that seem reasonable? Very open to suggestions and standardization here. Honestly, I've done a poor job of enforcing the sort of rigorous guarantees and expectations that people need in order to be able to rely on this in any sort of capacity, so would definitely like to clean this up.