Fixed MulVec code for Transpose case.

[btracey]

The previous code did not properly account for the shapes of the vectors. This makes the proper checks

[coveralls]

Coverage increased (+0.01%) when pulling 59ef3a7 on fixmulvec into c2b4ab1 on master.

[btracey]

@kortschak

Question: It is surprising to me that these tests pass. In the final test, an incorrectly-sized vector is being used as the receiver, but because it's equal to one of the input arguments, no panic occurs. I had followed the style of Dense, but now that I fully grok the consequences, couple of questions. If I'm reading the code right, these apply to both MulVec and Dense.

1. Right now, if the receiver is equal to one of the arguments, new data is created. Is that to avoid shadowing?
2. If m is equal to one of the arguments, then w is just the zero Vector/Dense. Shouldn't there be an additional check that the receiver size is the proper size? w may be a zero matrix, but m is not.
3. It doesn't seem like the current behavior plays properly with views (cap, grow, etc.). If m == b and m is a view of a matrix, new data is allocated and replaces the underlying data. It seems like the data should be copied into the old locations, thus preserving the correct stride information, etc.

[btracey]

Fixed code comments.
In the post above, the "last comment" is the one that is now commented out. The tests do pass with it commented out, even though it is the wrong size for some of the operations.

[coveralls]

Coverage decreased (-0.06%) when pulling 42aceb7 on fixmulvec into c2b4ab1 on master.

[kortschak]

1. Yes.
2. Yes, there should be an additional check. This will be likely incorrect in other operations that depend on a temporary workspace, w.
3. Yes, it does not. I guess we need to either find out if there is someway we can detect when something is a view and copy back in that case, always copy back or just warn in the documentation.

LGTM

[btracey]

On Jan 18, 2015 1:53 PM, "Dan Kortschak" notifications@github.com wrote:

Yes.
Yes, there should be an additional check. This will be likely incorrect
in other operations that depend on a temporary workspace, w.
Yes, it does not. I guess we need to either find out if there is someway
we can detect when something is a view and copy back in that case, always
copy back or just warn in the documentation.

We can tell from the capacities, right?

LGTM

—
Reply to this email directly or view it on GitHub.

[kortschak]

We can tell from the capacities, right?

A combination of the capCol and mat.Stride will tell if it's a view restricted by width, but I don't see a way to tell if it's restricted by height with the current information we keep.