Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix blas mat-vec multiplication on array with only 1 nontrivial dimension #585

Merged
merged 3 commits into from
Sep 4, 2019

Conversation

sebasv
Copy link
Contributor

@sebasv sebasv commented Jan 22, 2019

First attempt at fixing #584

@sebasv
Copy link
Contributor Author

sebasv commented Jan 23, 2019

The failing tests are behavior intended by this fix, needs discussion if this is desired behavior.

@sebasv sebasv changed the title fix array with only 1 nontrivial dimension Fix dot product on array with only 1 nontrivial dimension Jan 24, 2019
Copy link
Member

@jturner314 jturner314 left a comment

Choose a reason for hiding this comment

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

I'm sorry that it's taken me so long to review this PR. Thanks for your investigative work on this.

The only change I'd like to make is to add a comment explaining why those .max() calls are necessary.

@@ -588,8 +588,8 @@ pub fn general_mat_vec_mul<A, S1, S2, S3>(alpha: A,
{
let a_trans = CblasNoTrans;
let a_stride = match layout {
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add a comment, e.g.

                        // Determine stride between rows or columns. Note that the stride is
                        // adjusted to at least `k` or `m` to handle the case of a matrix with a
                        // trivial (length 1) dimension, since the stride for the trivial dimension
                        // may be arbitrary.

@jturner314 jturner314 added the bug label May 6, 2019
@bluss bluss changed the title Fix dot product on array with only 1 nontrivial dimension Fix blas mat-vec multiplication on array with only 1 nontrivial dimension Sep 3, 2019
@bluss bluss force-pushed the blas-gemv-error branch 2 times, most recently from b5cad2f to b101a27 Compare September 3, 2019 21:05
@bluss
Copy link
Member

bluss commented Sep 3, 2019

Using the maintainer push to PR to update.

I'll add your comment @jturner314 jturner314 but not in that way, because it causes a textual conflict.

@bluss
Copy link
Member

bluss commented Sep 3, 2019

@jturner314 @sebasv do you remember what the failing test comment was about? I guess it's obsolete now, because the tests pass and this looks ready to merge to me.

@jturner314
Copy link
Member

@jturner314 @sebasv do you remember what the failing test comment was about? I guess it's obsolete now, because the tests pass and this looks ready to merge to me.

I'm not sure. Maybe it's referring to the mat_vec_product_1d test added by the PR? (That would be surprising because the test was added after @sebasv made that comment, but maybe he just hadn't pushed the test yet?) That test fails before the fix in this PR.

@bluss
Copy link
Member

bluss commented Sep 4, 2019

ok must be obsolete, thanks :)

Thanks so much @sebasv for reporting and fixing! (It's me who renamed the test and the PR to avoid using the term "dot product" here.)

@bluss bluss merged commit 5e32de2 into rust-ndarray:master Sep 4, 2019
bluss added a commit that referenced this pull request Sep 4, 2019
@sebasv
Copy link
Contributor Author

sebasv commented Sep 4, 2019

Just for completeness, if I recall correctly there were two tests not related to this PR which failed because they called a dot product on a transposed array with a trivial dimension, which messed up the strides. If no tests fail now, this was probably resolved with another bugfix. @bluss @jturner314 thank you for your reviews and for merging the fix!

@bluss bluss mentioned this pull request Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants