TreB1eN / InsightFace_Pytorch

Pytorch0.4.1 codes for InsightFace
MIT License
1.73k stars 423 forks source link

Maybe a bug in pytorch 1.0 in computing the cos_theta in model.py #14

Closed tiandunx closed 5 years ago

tiandunx commented 5 years ago

Hi,thanks for your contribution. I've read your code carefully and found that there are few lines of code in model.py that really puzzled me. On line 275 in model.py, you wrote label = label.view(-1, 1), then on line 277, you wrote output[torch.range(0,nB-1).to(torch.long),label] = cos_theta_m[torch.range(0,nB-1).to(torch.long),label]. Here label is a 2-rank matrix, and cos_theta_m[torch.range(0, nB-1).to(torch.long), label] will choose the whole column of the orginial matrix instead of the very single element of the ground truth value. So it might overwrite other entries of the cos matrix whose label is NOT the column id.

TreB1eN commented 5 years ago

e... exactly the opposite, indexing this way will only update those outputs in each batch that corresponds to the ground truth label.

You can make up some random matrix to test and see the result.

tiandunx commented 5 years ago

Thanks for your quick reply. Here is my example. image As can be seen from above, x is a 4 x 6 matrix, and each row represents a sample. And there are 4 samples and 6 classes. y is another 4 x 6 matrix. Given the label that contains 4 elements. For sample 0, its label is 0, for sample 1, its label is 2. So the elements we want to modify are (0, 0), (1, 2), (2, 3), (3, 1), but after x[torch.arange(0,4), label] = y[torch.arange(0, 4), label] , the first column of x is changed which I only want to update (0, 0)

TreB1eN commented 5 years ago

yes, you are correct, we should remove this line:

label = label.view(-1, 1)

I have already updated it in the codes, thank you again 👍 @tiandunx