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

correct shape for vec*mat error #14679 #14961

Closed
wants to merge 1 commit into from

Conversation

fwang88
Copy link

@fwang88 fwang88 commented Feb 6, 2016

If the first dimension of a matrix is not 1, we should throw a dimension mismatch message for vec*mat.

@andreasnoack
Copy link
Member

The present definition will already give the error you ask for. It will just be thrown from the matmat method instead.

julia> randn(2)*randn(2,2)
ERROR: DimensionMismatch("A has dimensions (2,1) but B has dimensions (2,2)")
 in gemm_wrapper! at linalg/matmul.jl:312
 in * at linalg/matmul.jl:89

@fwang88
Copy link
Author

fwang88 commented Feb 6, 2016

This is from issue #14679. In your example, the dimension of A should be (2,), not (2,1).

@tkelman tkelman added the needs tests Unit tests are required for this change label Feb 7, 2016
@fwang88 fwang88 force-pushed the master branch 3 times, most recently from e1076f9 to 92ff0dd Compare February 8, 2016 19:57
…case

In addition, macro except_str is moved from test/replutil.jl to base/test.jl for more usage in tests.
@fwang88
Copy link
Author

fwang88 commented Feb 8, 2016

I added a test example. Since I'm quite new to all these, I'll appreciate a lot if any comments are provided.

@tkelman tkelman removed the needs tests Unit tests are required for this change label Feb 8, 2016
@tkelman
Copy link
Contributor

tkelman commented Feb 8, 2016

LGTM. Separate issue, but I wonder if using "A" and "B" in these error messages is maybe not the clearest...

@fwang88
Copy link
Author

fwang88 commented Feb 8, 2016

@tkelman Yes, I agree with you. But it seems the error message of any mismatched matrix multiplication uses "A" and "B", as used in function gemm_wrapper!

@tkelman
Copy link
Contributor

tkelman commented Feb 8, 2016

Yeah, it should be consistent here for now, but there's an opportunity to make both clearer in the future. Thanks for the contribution!

@oscardssmith
Copy link
Member

Fixed on current versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants