Adding CTC loss #342

maetshju · 2018-08-07T04:44:11Z

This is the CTC loss function and associated tests. it should interface with the tracker just like the other loss functions, and it uses the @require macro to allow the GPU functionality to be optional.

MikeInnes

Thanks a lot for this, and apologies for the long wait in reviewing. The main thing it needs now is some additions to the docs so that it's totally clear what this is for and how to use it.

MikeInnes · 2018-09-18T13:46:13Z

src/layers/ctc.jl

+end
+
+@require CUDAnative begin
+@require CuArrays begin


This can just be @require CuArrays, since CUDAnative will always be a dependency.

These blocks will also need an update for 1.0.

MikeInnes · 2018-09-18T13:47:44Z

src/layers/ctc.jl

+
+@grad function ctc(ŷ, y)
+  ls, gs = ctc(Flux.Tracker.data(ŷ), Flux.Tracker.data(y))
+  return ls, Δ -> (Δ .* gpu(gs), Δ)


Why is the gpu call needed here? We should find a different way to do this.

MikeInnes · 2018-09-18T13:49:00Z

test/layers/ctc.jl

+    @require CuArrays begin
+      using CuArrays
+      lossvalue = 3.6990738
+      l, gs = ctc(Flux.gpu(x), Flux.gpu(y))


Can we test the CPU version as well, and make sure the outputs are the same?

Worth also having a numerical gradcheck? Might not be necessary if we have known-good values.

maetshju · 2018-09-24T16:46:47Z

I should be able to get to this after a conference I'm going to this week. Doesn't look like it should be too much work; and I think it's a good idea to add it to the docs pages.

CTC functions updated to work with current versions of Flux, CuArrays, and CUDAnative. GPU functions split into a new file to allow conditional loading. Test cases using gradchecks developed.

GPU threads desynchronized on call to div when calculating gradients. Changing div to CUDAnative.div_fast keeps the threads synchronized.

MikeInnes · 2019-09-05T13:03:54Z

I wonder if the core kernel should be added to CuArrays so we can make sure that's well tested, and then we can add the interface parts to Flux separately. (@maleadt does that sound reasonable to you?)

maleadt · 2019-09-05T13:16:21Z

Unless there's an NNlib interface to implement with this functionality, I'd rather not. We should think about where to put functionality like that, and I think of CuArrays as implementing existing array-like interfaces for the GPU rather than housing "arbitrary" functionality.

MikeInnes · 2019-09-05T13:56:38Z

Seems fair.

@maetshju what's the status on this, is it ready on your end, or are you still hacking on it?

maetshju · 2019-09-05T16:23:45Z

@MikeInnes the core functionality should be there. I am going to make a pass through all the code over the weekend to do a few more tests and make sure the documentation/comments are sufficient and correct, but I think it should be good to go after that.

maetshju · 2019-09-09T06:13:24Z

I believe this is ready to go. It consistently passes the tests I've written, and I've checked that the methods properly cover all the 16 possible cases of tracked/untracked cu/non-cu array combinations. The docs should be up to date as well.

If it looks good on your end, would you like me to squash the commits together before merging?

DhairyaLGandhi · 2019-09-09T07:11:49Z

I'm going to do a quick run on the GPU also

bors try

bors · 2019-09-09T07:27:06Z

try

Build failed

ci/gitlab/trying

maetshju · 2019-09-10T16:27:16Z

I'm not sure I understand why that previous build failed. It doesn't look like Docker got everything installed before failing.

MikeInnes · 2020-01-15T16:55:16Z

We've occasionally had CI flakiness, so let's just try again:

bors try

bors · 2020-01-15T16:55:20Z

try

Merge conflict

maetshju · 2020-01-19T04:41:03Z

These last few commits fix the merge conflict, and I think they get everything working with Zygote. Admittedly, I'm not super familiar with Zygote yet, but the CPU tests are passing. I haven't had a chance to check on GPU yet though.

MikeInnes · 2020-01-20T15:42:46Z

Thanks a lot. Will give the GPU tests a quick go to see where we are:

bors try

bors · 2020-01-20T15:45:44Z

try

Build failed

ci/gitlab/gitlab.com

maetshju · 2020-01-21T03:20:36Z

That most recent commit should fix the UndefVarError by using CuArrays.functional(). It's passing tests in my GPU environment, so hopefully it works with the CI.

MikeInnes · 2020-01-21T14:04:57Z

bors try

bors · 2020-01-21T14:21:14Z

try

Build succeeded

ci/gitlab/gitlab.com

bhvieira · 2020-01-27T21:32:49Z

Manifest.toml

@@ -74,6 +80,12 @@ git-tree-sha1 = "efdaf19ab11c7889334ca247ff4c9f7c322817b0"
 uuid = "bbf7d656-a473-5ed7-a52c-81e309532950"
 version = "0.2.0"

+[[Conda]]


Is this necessary? I haven't followed the PR, though.

That's a good catch. The dependency was induced by FFTW's version 1.1.0, which in turn was induced by its MKL_jll and FFTW_jll dependencies requiring Julia 1.3, while I was running 1.1. I upgraded my version of Julia and then updated packages, which should remove the dependency on Conda.

It should still install on older versions of Julia now, since it can pull in the older version of FFTW that requires Conda. Or at least, I was able to install this new version from the pull branch on Julia 1.1.

I think so. Couldn't imagine FFTW would induce the Conda dependency indirectly, will keep that in mind!

CarloLucibello · 2020-07-02T16:22:31Z

@maetshju it is a pity if this gets lost. There are two changes impacting this PRs, #1258 and #1264 . If you will have the patience to rebase once #1264 gets in, we can have a quick review and merge

CarloLucibello · 2020-07-02T16:23:19Z

starting a new PR all over may be easier than rebase

maetshju · 2020-07-02T17:39:38Z

@CarloLucibello Thanks for letting me know about those PRs. I think opening a new PR once the others have been merged is probably easier, as you suggested. I would love to get this committed; it's been lingering in my mind for a while.

1287: Add CTC loss to new Losses module r=CarloLucibello a=maetshju This is a redux of adding the connectionist temporal classification loss from #342, now that the Losses module has been merged in #1264. Discussion in #342 suggested that a new PR would be easier than rebasing. Since the last commit in #342, functions and data structures from `CUDAnative.jl` and `CuArrays.jl` have been updated to work with `CUDA.jl`. This is in addition to incorporating the loss function into the Losses module. ### PR Checklist - [X] Tests are added - [X] Entry in NEWS.md - [X] Documentation, if applicable - [ ] Final review from `@dhairyagandhi96` (for API changes). Co-authored-by: Matt Kelley <matthew.curtis.kelley@gmail.com> Co-authored-by: Matthew C. Kelley <matthew.curtis.kelley@gmail.com>

maetshju · 2021-01-20T17:22:24Z

Superseded by #1287.

MikeInnes force-pushed the master branch from 65799d1 to 193c4de Compare September 5, 2018 15:53

MikeInnes reviewed Sep 18, 2018

View reviewed changes

jtmatamalas mentioned this pull request Nov 12, 2018

Tied Weights #488

Closed

maetshju added 10 commits September 2, 2019 20:56

Add CTC loss and related tests

eb0f4c1

Update ctc test

3eedec3

Update CTC test loss vlaues

b8304cf

Use 0.6 @cuda syntax

e212ab7

Make ctc tests a testset

5e3820c

Quotes around CTC testset

aff00c4

Add extra @require

692feaa

Add extra @require

12d9ca2

Added @require CuArrays

29162b8

Update @cuda call to 0.7 syntax

0ddb097

maetshju force-pushed the addctc branch from f6f4a59 to 0ddb097 Compare September 3, 2019 02:59

maetshju added 2 commits September 2, 2019 22:09

Update for contemporary Flux, CuArrays, and CUDAnative packages

2cc87fc

CTC functions updated to work with current versions of Flux, CuArrays, and CUDAnative. GPU functions split into a new file to allow conditional loading. Test cases using gradchecks developed.

Fix gpu thread synchroniztion

832e6b1

GPU threads desynchronized on call to div when calculating gradients. Changing div to CUDAnative.div_fast keeps the threads synchronized.

maetshju added 8 commits September 8, 2019 16:51

Add test cases with hand-calculated values

5dea96c

Fix typo in function call

f0c5302

Update docs for ctc

96505ab

Fix linebreaks on ctc docstring

4c9b444

Fix indexing error in ctc testcases

57813da

Typo in docs/make.jl

c96feaa

Add methods to ctc and ctc_ functions for type coverage

0fdf7e0

Remove boilerplates on ctc and ctc_ methods

1822c88

bors bot added a commit that referenced this pull request Sep 9, 2019

Try #342:

a93fc62

maetshju added 3 commits January 18, 2020 20:18

Fix merge conflict

04d94bb

Update to Zygote API

ed2f820

Add LinearAlgebra as dependency in project environment

de4ca94

bors bot added a commit that referenced this pull request Jan 20, 2020

Try #342:

e4ed656

Replace has_cuarrys() with CuArrays.functional()

5ae84fc

bors bot added a commit that referenced this pull request Jan 21, 2020

Try #342:

d6636ed

bhvieira reviewed Jan 27, 2020

View reviewed changes

Update package dependencies to remove Conda dependency

ca76e5b

DhairyaLGandhi mentioned this pull request Apr 21, 2020

CTC loss #1135

Closed

nrxszvo mentioned this pull request May 27, 2020

sync_threads() appears to not be sync'ing threads JuliaGPU/CUDA.jl#61

Closed

maetshju mentioned this pull request Jul 21, 2020

Add CTC loss to new Losses module #1287

Merged

4 tasks

maetshju closed this Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding CTC loss #342

Adding CTC loss #342

maetshju commented Aug 7, 2018

MikeInnes left a comment

MikeInnes Sep 18, 2018

MikeInnes Sep 18, 2018

MikeInnes Sep 18, 2018

maetshju commented Sep 24, 2018

MikeInnes commented Sep 5, 2019

maleadt commented Sep 5, 2019

MikeInnes commented Sep 5, 2019

maetshju commented Sep 5, 2019

maetshju commented Sep 9, 2019

DhairyaLGandhi commented Sep 9, 2019

bors bot commented Sep 9, 2019

maetshju commented Sep 10, 2019

MikeInnes commented Jan 15, 2020

bors bot commented Jan 15, 2020

maetshju commented Jan 19, 2020

MikeInnes commented Jan 20, 2020

bors bot commented Jan 20, 2020

maetshju commented Jan 21, 2020

MikeInnes commented Jan 21, 2020

bors bot commented Jan 21, 2020

bhvieira Jan 27, 2020

maetshju Jan 28, 2020

maetshju Jan 28, 2020

bhvieira Jan 28, 2020

CarloLucibello commented Jul 2, 2020

CarloLucibello commented Jul 2, 2020

maetshju commented Jul 2, 2020 •

edited

Loading

maetshju commented Jan 20, 2021

Adding CTC loss #342

Adding CTC loss #342

Conversation

maetshju commented Aug 7, 2018

MikeInnes left a comment

Choose a reason for hiding this comment

MikeInnes Sep 18, 2018

Choose a reason for hiding this comment

MikeInnes Sep 18, 2018

Choose a reason for hiding this comment

MikeInnes Sep 18, 2018

Choose a reason for hiding this comment

maetshju commented Sep 24, 2018

MikeInnes commented Sep 5, 2019

maleadt commented Sep 5, 2019

MikeInnes commented Sep 5, 2019

maetshju commented Sep 5, 2019

maetshju commented Sep 9, 2019

DhairyaLGandhi commented Sep 9, 2019

bors bot commented Sep 9, 2019

try

Build failed

maetshju commented Sep 10, 2019

MikeInnes commented Jan 15, 2020

bors bot commented Jan 15, 2020

try

Merge conflict

maetshju commented Jan 19, 2020

MikeInnes commented Jan 20, 2020

bors bot commented Jan 20, 2020

try

Build failed

maetshju commented Jan 21, 2020

MikeInnes commented Jan 21, 2020

bors bot commented Jan 21, 2020

try

Build succeeded

bhvieira Jan 27, 2020

Choose a reason for hiding this comment

maetshju Jan 28, 2020

Choose a reason for hiding this comment

maetshju Jan 28, 2020

Choose a reason for hiding this comment

bhvieira Jan 28, 2020

Choose a reason for hiding this comment

CarloLucibello commented Jul 2, 2020

CarloLucibello commented Jul 2, 2020

maetshju commented Jul 2, 2020 • edited Loading

maetshju commented Jan 20, 2021

maetshju commented Jul 2, 2020 •

edited

Loading