Skip to content

Conversation

@zh217
Copy link
Contributor

@zh217 zh217 commented Jun 18, 2019

The bug is that when target_length == 0, there is no preceding BLANK state and the original implementation will lead to out of bound pointer access.

@t-vi
Copy link
Collaborator

t-vi commented Jun 18, 2019

Thank you!

Looks good at first sight, could you add a test (under test/test_nn.py; or amend an existing one) and maybe also add a small comment in the code? Thanks!

@pytorchbot pytorchbot added the module: nn Related to torch.nn label Jun 18, 2019
@zh217
Copy link
Contributor Author

zh217 commented Jun 18, 2019

Done.

Copy link
Collaborator

@t-vi t-vi left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

@t-vi
Copy link
Collaborator

t-vi commented Jun 23, 2019

@pytorchbot merge this please

@pytorchbot pytorchbot added the merge-this-please Was marked for merge with @pytorchbot merge this please label Jun 23, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jun 24, 2019
Summary:
The bug is that when target_length == 0, there is no preceding BLANK state and the original implementation will lead to out of bound pointer access.
Pull Request resolved: pytorch/pytorch#21910

Differential Revision: D15960239

Pulled By: ezyang

fbshipit-source-id: 7bbbecb7bf91842735c14265612c7e5049c4d9b3
@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 0ac28c8.

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

Labels

merge-this-please Was marked for merge with @pytorchbot merge this please Merged module: nn Related to torch.nn open source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants