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

The masks are all equal to 1 #13

Open
zxw866 opened this issue Oct 28, 2019 · 3 comments
Open

The masks are all equal to 1 #13

zxw866 opened this issue Oct 28, 2019 · 3 comments

Comments

@zxw866
Copy link

zxw866 commented Oct 28, 2019

result = []
for item in items:

    if len(item) < l:

        item = [1. for _ in item] + ([0.] * (l - len(item)))

    if len(item) >= l:

        item = [1. for _ in item[:l]]
    result.append(item)

in data.py, line 148.

This code causes the masks to be all equal to 1.0 ? Is there a problem here?

@seanmacavaney
Copy link
Contributor

Ah, yes it seems so. The problem arose from re-using the item variable. Thanks!

In [1]: import torch                                                                                                                                                                                        

# BEFORE

In [2]:  
   ...: def _mask(items, l): 
   ...:     result = [] 
   ...:     for item in items: 
   ...:         if len(item) < l: 
   ...:             item = [1. for _ in item] + ([0.] * (l - len(item))) 
   ...:         if len(item) >= l: 
   ...:             item = [1. for _ in item[:l]] 
   ...:         result.append(item) 
   ...:     return torch.tensor(result).float().cuda() 
   ...:                                                                                                                                                                                                     
In [3]: items = [[1,2,3,4,5,6], [1,2,3], [1], []]
In [4]: _mask(items, 3)                                                                                                                                                                                     
Out[4]: 
tensor([[1., 1., 1.],
        [1., 1., 1.],
        [1., 1., 1.],
        [1., 1., 1.]], device='cuda:0')

# AFTER

In [5]: 
    ...: def _mask(items, l): 
    ...:     result = [] 
    ...:     for item in items: 
    ...:         if len(item) < l: 
    ...:             mask = [1. for _ in item] + ([0.] * (l - len(item))) 
    ...:         if len(item) >= l: 
    ...:             mask = [1. for _ in item[:l]] 
    ...:         result.append(mask) 
    ...:     return torch.tensor(result).float().cuda() 
In [6]: _mask(items, 3)                                                                                                                                                                                    
Out[6]: 
tensor([[1., 1., 1.],
        [1., 1., 1.],
        [1., 0., 0.],
        [0., 0., 0.]], device='cuda:0')

seanmacavaney added a commit that referenced this issue Oct 28, 2019
As brought up in #13, the previous implementation always returns 1 for every element. This was due to the re-use of the item variable. This change should make the
@cmacdonald
Copy link
Contributor

Sean, can you comment on which cases this will affect, and if effectiveness will be markedly affected?

@seanmacavaney
Copy link
Contributor

This effects cases in which the batch is larger than 1. With the default settings, this is the case for training (gradient accumulation batch size of 2) and evaluation (batch size of 16). It will have the most effect when document lengths differ considerably (i.e., a very short document in a batch with a long document).

As far as the effectiveness goes, I will need to run some experiments to test this. Since only padding tokens are masked, I imagine that the models should learn to ignore these tokens. But the only way to know with certainty is to verify it empirically.

@seanmacavaney seanmacavaney reopened this Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants