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

Simplifies Subsequent Mask #2224

Merged
merged 4 commits into from Dec 21, 2018

Conversation

Projects
None yet
3 participants
@vidurj
Copy link
Contributor

commented Dec 21, 2018

The subsequent_mask function in the bidirectional_language_model_transformer seemed to be unnecessarily convoluted.

@vidurj vidurj requested a review from brendan-ai2 Dec 21, 2018

mask = np.triu(np.ones(attn_shape), k=1).astype('uint8')
mask = (torch.from_numpy(mask) == 0)
return mask.to(device)
mask = torch.tril(torch.ones(size, size, device=device)).unsqueeze(0)

This comment has been minimized.

Copy link
@brendan-ai2

brendan-ai2 Dec 21, 2018

Member

Shouldn't the type of uint8 be preserved?

This comment has been minimized.

Copy link
@vidurj

vidurj Dec 21, 2018

Author Contributor

The old return type was bool because of torch.from_numpy(mask) == 0. At the only place this is being used in the code base currently we have subsequent = subsequent_mask(timesteps, device).int() so the return type doesn't matter. I could cast to int to match this use case if that's preferable.

@brendan-ai2

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

Thanks for creating things as int directly. Just needs a small lint fix: W: 15, 0: Unused numpy imported as np (unused-import)

@brendan-ai2
Copy link
Member

left a comment

After linter passes :)

vidur-joshi added some commits Dec 21, 2018

@vidurj

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Thanks. Merging.

@vidurj vidurj merged commit e2f66c0 into master Dec 21, 2018

3 checks passed

Pull Requests (AllenNLP Library) TeamCity build finished
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project Absolute coverage decreased by -<1% but relative coverage increased by +7% compared to ce060ba
Details

@vidurj vidurj deleted the simplify-subsequent-mask branch Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.