Skip to content

Add failing tests for concat/unconcat ops#406

Draft
MarkKremer wants to merge 4 commits intogorgonia:masterfrom
MarkKremer:debug-unconcat-concat
Draft

Add failing tests for concat/unconcat ops#406
MarkKremer wants to merge 4 commits intogorgonia:masterfrom
MarkKremer:debug-unconcat-concat

Conversation

@MarkKremer
Copy link
Copy Markdown
Contributor

@MarkKremer MarkKremer commented Jun 1, 2020 •
edited
Loading

Added some failing tests for something that I expected to work and I need to build a LSTM layer. One of the errors I get is:

=== RUN   TestUnconcatConcatOpSequence
--- FAIL: TestUnconcatConcatOpSequence (0.00s)
    op_tensor_test.go:449: "Reshape" not yet implemented for non-contiguous views
        gorgonia.org/tensor.(*Dense).Reshape
        	/home/markkremer/go/pkg/mod/gorgonia.org/[email protected]/dense.go:146
        gorgonia.org/gorgonia.reshapeOp.UnsafeDo
        	..../Go/src/github.com/MarkKremer/gorgonia/op_tensor.go:1205
        gorgonia.org/gorgonia.(*execOp).exec
        	..../Go/src/github.com/MarkKremer/gorgonia/vm_tape_nocuda.go:52
        gorgonia.org/gorgonia.(*tapeMachine).runall
        	..../Go/src/github.com/MarkKremer/gorgonia/vm_tape.go:236
        runtime.goexit
        	/usr/local/go/src/runtime/asm_amd64.s:1357
        Failed to carry UnsafeDo()
        gorgonia.org/gorgonia.(*execOp).exec
        	..../Go/src/github.com/MarkKremer/gorgonia/vm_tape_nocuda.go:53
        gorgonia.org/gorgonia.(*tapeMachine).runall
        	..../Go/src/github.com/MarkKremer/gorgonia/vm_tape.go:236
        runtime.goexit
        	/usr/local/go/src/runtime/asm_amd64.s:1357
        PC 12. Failed to execute instruction Reshape(2, 3, 1)	[CPU3]	CPU3	false	true	false
        gorgonia.org/gorgonia.(*tapeMachine).runall
        	..../Go/src/github.com/MarkKremer/gorgonia/vm_tape.go:237
        runtime.goexit
        	/usr/local/go/src/runtime/asm_amd64.s:1357
        PC: 12
        gorgonia.org/gorgonia.(*tapeMachine).RunAll
        	..../Go/src/github.com/MarkKremer/gorgonia/vm_tape.go:220
        gorgonia.org/gorgonia.TestUnconcatConcatOpSequence
        	..../Go/src/github.com/MarkKremer/gorgonia/op_tensor_test.go:448
        testing.tRunner
        	/usr/local/go/src/testing/testing.go:909
        runtime.goexit
        	/usr/local/go/src/runtime/asm_amd64.s:1357
FAIL

What I noticed is:

  • The concat op's SymDiff method doesn't always return a gradient with the same shape as the input nodes. This will cause problems when using that gradient in other ops.
  • The compiler (compile.go -> codegenerator.addNode(...)) determines that the reshape can be done using the UnsafeDo method. But Unconcat returns a view which isn't supported by that method. I'm not entirely sure if we could just copy the if v.IsView() { val = v.Materialize() } code from the Do() method. Or should the compiler have a check instead that uses the safe Do() method if one or more of the inputs are views?

This is slightly related to #404 in that I need it for the same model and that something with the shapes is going wrong. But now that I have found the issues it doesn't have much overlap that is of interest for Gorgonia.

@chewxy
Copy link
Copy Markdown
Member

chewxy commented Jun 1, 2020

I think this is fixed in #391

@chewxy
Copy link
Copy Markdown
Member

chewxy commented Jun 1, 2020

.... which I just noticed is not merged.

@MarkKremer
Copy link
Copy Markdown
Contributor Author

MarkKremer commented Jun 1, 2020

That would be great. I'll check if my tests work with that.

@MarkKremer
Copy link
Copy Markdown
Contributor Author

MarkKremer commented Jun 1, 2020

It fixes one of my test and I'm very happy that that part works now. TestConcatOp does still fail. And I keep getting this error in my own project:

ReduceAdd failed during differentiation: Operation failed: Failed to infer shape. Op: + false: Shape mismatch: (30, 128, 1) and (30, 128)

After TestConcatOp is fixed this one might be resolved too. But I'll try to create another test which actually produces the error above. Extra info: ReduceAdd is called in differentiation.go.

What needs to happen before #391 can get merged?

@MarkKremer
Copy link
Copy Markdown
Contributor Author

MarkKremer commented Jun 1, 2020

I added a test that gives the error I mentioned. ReduceAdd used here doesn't support adding tensors of different shapes. Something else should be used or the grads have to be broadcasted at some point before reaching this code. But this is only necessary if there are more that 2 gradients to be applied, and those gradients have incompatible shapes.

My TestConcatOp checks the gradient shapes but now that I think of it, I'm not certain it will cause any actual problems if the other problems get fixed. It depends on if the other pathways that apply the gradients can deal with slightly different shapes (e.g. (3, 3, 1) vs (3, 3)). I don't know if that's the case.

@chewxy
Copy link
Copy Markdown
Member

chewxy commented Jun 1, 2020

I just merged #391 . Should I tag this version?

@MarkKremer
Copy link
Copy Markdown
Contributor Author

MarkKremer commented Jun 1, 2020

I don't care about it personally but it may be nice for the issue about "Convnet isn't learning".

@MarkKremer MarkKremer marked this pull request as draft June 2, 2020 10:26
@MarkKremer
Copy link
Copy Markdown
Contributor Author

MarkKremer commented Jun 7, 2020

Small update: I'll try to get my model working completely before finishing up this PR so I know it works as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MarkKremer @chewxy