Update documentation for CTCLoss #18415

rlorigro · 2019-03-25T02:18:14Z

This is meant to resolve #18249, where I pointed out a few things that could improve the CTCLoss docs.

My main goal was to clarify:

Target sequences are sequences of class indices, excluding the blank index
Lengths of target and input are needed for masking unequal length sequences, and do not necessarily = S, which is the length of the longest sequence in the batch.

I thought about Thomas's suggestion to link the distill.pub article, but I'm not sure about it. I think that should be up to y'all to decide.

I have no experience with .rst, so it might not render as expected :)

@t-vi @ezyang

t-vi · 2019-03-25T07:50:32Z

torch/nn/modules/loss.py

-        targets: Tensor of size :math:`(N, S)` or `(sum(target_lengths))`.
-            Targets (cannot be blank). In the second form, the targets are assumed to be concatenated.
+        targets: Tensor of size :math:`(N, S)` or `(sum(target_lengths))`, where `N = batch size`, and
+        `S = maximum target length`.


What's the effect of changing the indent here?

Did you mean on line 1238?

The indent means that the remaining words in that sentence will belong to the header block when rendered. I noticed that some of the other docs don't do this, so I have actually changed it again to something else.

t-vi · 2019-03-25T07:52:38Z

torch/nn/modules/loss.py

+            such that :math: `target_n = targets[n,:s_n]` for each target in a batch.
        target_lengths: Tuple or tensor of size  :math:`(N)`.
-            Lengths of the targets
+            Lengths of the targets (may be :math: < S)


Maybe should be <= S if targets is a two-dimensional tensor (but with :math:) would be clearer.

t-vi · 2019-03-25T07:53:56Z

torch/nn/modules/loss.py

+        >>>
+        >>> min_target_length = 10
+        >>> max_target_length = S
+        >>>


I must admit that the blank lines are just too much. Also, I wonder, if we need to introduce a second name for S.

I redid the naming it in the latest update. Also removed some of the self explanatory comments, and removed some spaces.

t-vi · 2019-03-25T07:55:25Z

Thank you for your patch! It will be nice to have the improved documentation.
I have a few minor comments. - Also did you build the documentation to see how it is rendered after your changes?

ramithp · 2019-03-25T14:01:57Z

torch/nn/modules/loss.py

+        targets: Tensor of size :math:`(N, S)` or `(sum(target_lengths))`, where `N = batch size`, and
+        `S = maximum target length`.
+            Each element in the target sequence is a class index. Target index cannot be blank (default=0).
+            In the second form, the targets are assumed to be concatenated.


Are the targets concatenated with or without the padding to make it a fixed (N, S)?
Maybe it would be useful to explicitly comment on that.

I think the answer is "with padding". I actually still don't understand how the (sum(target_lengths)) form works... it doesn't seem like that actually describes a shape. Can @t-vi comment on this?

That's a 1-d shape. (sum(target_lengths),) would probably be more pythonesque.

Ah no I see now. Not sure why I was confused before. The comma is probably not necessary.

rlorigro · 2019-03-26T04:25:19Z

Ok I made some changes, hopefully for the better.

Since you asked about rendering, I didn't want to try to build this because of the dependencies and errors I got when I tried, but i did find a cheap renderer online (missing the math plugin) to give a sense of the appearance

and the example now looks like this:

t-vi

Thank you for checking the rendering! (I had half-hoped that you'd go through the trouble of installing the doc dependencies and make a habit of improving those things you find imperfect. 😉)

From my point of view, this can go in, unless you want to do something about the less than perfect explanation of concatenated targets.

rlorigro · 2019-03-26T06:55:58Z

No problem. I will make one more edit tomorrow to address the lengths/concatenation, since there is no rush. Also, does the alternative concatenated form not also apply to inputs?

t-vi · 2019-03-26T07:06:28Z

I will make one more edit tomorrow to address the lengths/concatenation, since there is no rush.
Great, thanks!

Also, does the alternative concatenated form not also apply to inputs?
No.

ezyang · 2019-03-26T16:47:32Z

OK, I'll wait for the last change before landing this

rlorigro · 2019-03-28T03:15:25Z

@ezyang @t-vi

Ok here is the final edit. It is still less than perfect, but a lot less less than perfect than it was before. I decided to just explicitly break out the explanations for both forms of input. I used the term "stacked" vs "concatenated" because it kind of makes sense considering the difference between these 2 operations in torch.

This is how it renders (roughly):

Looks like it says there is now a conflict in loss.py. not sure what that is caused by.

t-vi · 2019-03-28T07:34:48Z

Thanks! Two quick comments:

In the explanation of target lengths, you write <= T when it should be <= S. I missed that earlier, sorry!
I must admit I don't understand the explanation for concatenated lengths. If the targets are given as a 1d tensor that is the concatenation of individual targets, the target_lengths must add up to the total length of the tensor. maybe?

rlorigro · 2019-03-28T18:44:23Z

Ah sorry about that, I will fix it, and use your wording for target_lengths

rlorigro · 2019-03-29T21:31:51Z

@t-vi @ezyang

Documentation should be fixed now

t-vi · 2019-03-29T21:50:16Z

Looks good! Thanks! But there is a merge conflict for the formatting of the reduction strings.

soumith · 2019-03-29T22:43:20Z

I fixed the merge conflict. Thanks a lot @rlorigro !

facebook-github-bot

@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-03-30T08:32:55Z

@soumith merged this pull request in 96456bf.

rlorigro · 2019-04-03T21:58:35Z

Thanks @soumith for fixing the conflict. Does this page get updated whenever the next version of pytorch is officially released: https://pytorch.org/docs/stable/nn.html#ctcloss ?

soumith · 2019-04-03T21:59:16Z

@rlorigro yes. right now master docs are already updated: https://pytorch.org/docs/master/nn.html#ctcloss

rlorigro · 2019-04-03T22:02:20Z

Awesome, thanks. Ah it looks like the kerning on the "math" font is making it almost unreadable in some places. That was unfortunately not rendered in the renderer I used for testing.

Also the underscore is being parsed as a subscript in target_lengths

edchengg · 2019-05-13T19:47:25Z

why is the input size for class is C = 20 which excluding blank? If the network does not predict the blank in the output, why would this loss make sense...

# Initialize random batch of input vectors, for *size = (T,N,C) input = torch.randn(T, N, C).log_softmax(2).detach().requires_grad_()
????

instead of

# Initialize random batch of input vectors, for *size = (T,N,C) input = torch.randn(T, N, C + 1).log_softmax(2).detach().requires_grad_()

rlorigro · 2019-05-13T20:10:32Z

I think you are right, it should actually be C+1 for input and and C for target

rlorigro added 4 commits March 24, 2019 18:47

Updated CTCLoss documentation for clarity

ac1a6e2

Updated CTCLoss documentation

09e72ba

Updated CTCLoss documentation (reworded)

da3cbe1

Updated CTCLoss documentation (more info)

a14419e

t-vi reviewed Mar 25, 2019

View reviewed changes

ramithp reviewed Mar 25, 2019

View reviewed changes

Updated CTCLoss documentation (made prettier, clearer)

198f2dd

Updated CTCLoss documentation (replaced length requirement)

d0e59d9

t-vi approved these changes Mar 26, 2019

View reviewed changes

Updated CTCLoss documentation (more info about targets)

9cbd097

Updated CTCLoss documentation (fixed error)

a5a8516

Merge branch 'master' into master

9a05ad4

facebook-github-bot reviewed Mar 29, 2019

View reviewed changes

facebook-github-bot closed this in 96456bf Mar 30, 2019

facebook-github-bot added the merged label Mar 30, 2019

ezyang added the open source label Jun 24, 2019

Update documentation for CTCLoss #18415

Update documentation for CTCLoss #18415

Uh oh!

Conversation

rlorigro commented Mar 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-vi commented Mar 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlorigro commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-vi left a comment

Choose a reason for hiding this comment

Uh oh!

rlorigro commented Mar 26, 2019

Uh oh!

t-vi commented Mar 26, 2019

Uh oh!

ezyang commented Mar 26, 2019

Uh oh!

rlorigro commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-vi commented Mar 28, 2019

Uh oh!

rlorigro commented Mar 28, 2019

Uh oh!

rlorigro commented Mar 29, 2019

Uh oh!

t-vi commented Mar 29, 2019

Uh oh!

soumith commented Mar 29, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 30, 2019

Uh oh!

rlorigro commented Apr 3, 2019

Uh oh!

soumith commented Apr 3, 2019

Uh oh!

rlorigro commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edchengg commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlorigro commented May 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rlorigro commented Mar 26, 2019 •

edited

Loading

rlorigro commented Mar 28, 2019 •

edited

Loading

rlorigro commented Apr 3, 2019 •

edited

Loading

edchengg commented May 13, 2019 •

edited

Loading