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

Copy input shape for batchedMatMulOp instead of append #405

Merged
merged 3 commits into from Jun 3, 2020

Conversation

MarkKremer
Copy link
Contributor

@MarkKremer MarkKremer commented May 31, 2020

If you look a bit up in the diff of op_math.go you'll see outerX := x[:len(x)-2]. Taking a range from a slice and then appending to it will override the original slice instead of copying it into a new slice. Not very obvious but that's how it works. This change does a copy so that the shape of the input node doesn't get altered.

I looked into slice.Clone() but returning a slice to the pool that I appended to didn't feel right. Manually calling BorrowInts() creates a lot of clutter. It would be nice if there was a function to clone+append in the tensor package to do this. But for now I chose this approach. However, it isn't clear if outerX can contain more than 1 element. If it doesn't, then I could simplify it some more.

This is a result of #404 but don't close it yet. There are still some other issues I'm having that I'm still figuring out.

@coveralls
Copy link

coveralls commented May 31, 2020

Coverage Status

Coverage increased (+0.2%) to 64.523% when pulling dd90e4c on MarkKremer:fix-batched-mat-mul-op into 3eb51b4 on gorgonia:master.

@MarkKremer
Copy link
Contributor Author

#391 fixes this too. I propose I remove my fix from this branch but keep the test. Then you'll have some extra tests for your own fix. :)

@chewxy
Copy link
Member

chewxy commented Jun 1, 2020

agreed.. let me do some finnacking around the gomod things. (#391 relies on a newer version of tensor)

@chewxy
Copy link
Member

chewxy commented Jun 1, 2020

Will merge this into master once CI says go

@chewxy chewxy merged commit 51690a0 into gorgonia:master Jun 3, 2020
@MarkKremer MarkKremer deleted the fix-batched-mat-mul-op branch June 3, 2020 09:16
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.

None yet

3 participants