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

Added SubArray tests for linalg/matmul.jl and fixed Vector -> StridedVector in dot #15410

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented Mar 8, 2016

First of all: I know there are many commits. They are to be squashed, but they are there for my own sake until this is (hopefully) ready to be merged.

I've generally tried to do what I did in #15364, #15354, #15314, but I tried not to close my eyes and add for Atype in ["Array", "SubArray"] everywhere without thinking. Some cases I ignored were the ones already testing SubArray related problems, and also the gemv! tests. The latter was mostly due to the fact that gemv! is defined for Strided*, and the number of combinations of matrices of Array and SubArray is large. My concern was that these tests would take much longer to run, but for no real reason. I would love comments on this.

I also added let blocks around groups of tests with named matrices and vectors (A, B, ..., b, ...). This is because running all the tests in julia/test/ defines a lot of variables that can potentially be used by mistake if all contributors are not equally careful or lucky. At some point I had removed a A = definition somewhere, and another A was used by mistake (and didn't cause a test fail by chance). This is not ideal, as there's a chance such a mistake is not caught. If people don't like them, I can remove them again.

Other points to be made

  • The very first commit removes a 10 times loop, bit i is never used, so I removed it. Was there a reason simply couldn't infer from the test?
  • In commit 4, vf was constructed, but an old v was used
  • fixed dot such that it is defined for StridedVector instead of Vector This was not a good idea, and has been reverted.

@kshyatt kshyatt added test This change adds or pertains to unit tests linear algebra Linear algebra labels Mar 9, 2016
@@ -5,189 +5,279 @@ using Base.Test
## Test Julia fallbacks to BLAS routines

# matrices with zero dimensions
for i = 1:10
Copy link
Contributor

Choose a reason for hiding this comment

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

blame says @andreasnoack 687d2b0 - maybe because Array would be uninitialized in the non-zero-dimensional case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I'm not quite sure if I understand why you would have to loop ten times in that case either, but I'll look at it again.

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember why I did that (a comment would probably have helped), but my guess is what @tkelman said. When multiplying an mx0 matrix with an 0xn then we have a fast branch that just allocates an mxn matrix and sets the elements to zero. Repeating the tests makes it more likely to catch it if we forgot to set the memory to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I get it. Do you want a loop again then, but with a comment?

Copy link
Member

Choose a reason for hiding this comment

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

I think it fine to remove the loop. We should see occational test failures even without it if the arrays are not set correctly.

@andreasnoack
Copy link
Member

@pkofod Please rebase when you have the time. Except for the dot comment, I think this ready.

@pkofod pkofod force-pushed the matmul branch 2 times, most recently from bc33b64 to 18a2ddb Compare April 11, 2016 13:41
@pkofod
Copy link
Contributor Author

pkofod commented Apr 11, 2016

@andreasnoack tests pass; should I squash to one commit?

edit: I tried to look for the other dot method, but as far as I could see, they didn't overlap, see the comment to your comment.

@andreasnoack
Copy link
Member

I think two commits are fine here, but please see my reply to your reply to my comment regarding the dot method.

@pkofod pkofod force-pushed the matmul branch 2 times, most recently from bc33b64 to 26c03de Compare April 25, 2016 07:27
@pkofod
Copy link
Contributor Author

pkofod commented Apr 25, 2016

Rebased and pending tests. I reverted the changes to dot, as it didn't make sense. The point was to make the tests under the line # issue #6450 run for SubArrays, but it is not obvious that we want those functions to be tested for SubArrays. Instead, dot should be called without the ranges, and a new sub can be created if different ranges are wanted (instead of using dot(vec, range, vec, range)).

Edit
Just to be clear, I had allowed for

a, b = rand(30), rand(30)
as = sub(a, 1:5)
bs = sub(b, 1:5)
dot(as, bs)
# allowed
dot(as, 1:3, bs, 1:3)
# allowed

But it is not obvious why we would provide the rhs of ==. Now we have (again)

a, b = rand(30), rand(30)
as = sub(a, 1:5)
bs = sub(b, 1:5)
dot(as, bs)
# allowed
dot(as, 1:3, bs, 1:3)
# not allowed
# instead do
dot(a, 1:3, b, 1:3)
# or create new subs:
as2 = sub(a, 1:3)
bs2 = sub(b, 1:3)
dot(as2, bs2)

@andreasnoack
Copy link
Member

Yes. I think that is the right way to do it. Thanks.

@andreasnoack andreasnoack merged commit 2c26a0a into JuliaLang:master Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants