[GitHub] chaoyuaw commented on a change in pull request #10857: Fixed divison by zero bug in DistanceWeightedSampling in gluon example

[GitBox <gi...@apache.org>]

chaoyuaw commented on a change in pull request #10857: Fixed divison by zero bug in DistanceWeightedSampling in gluon example
URL: https://github.com/apache/incubator-mxnet/pull/10857#discussion_r187081528
 
 

 ##########
 File path: example/gluon/embedding_learning/model.py
 ##########
 @@ -110,7 +110,10 @@ def hybrid_forward(self, F, x):
             mask[i:i+k, i:i+k] = 0
 
         weights = weights * F.array(mask) * (distance < self.nonzero_loss_cutoff)
-        weights = weights / F.sum(weights, axis=1, keepdims=True)
+        weight_norm = F.sum(weights, axis=1, keepdims=True)
+        # we need to change all zero elements to 1 to avoid division by zero
+        weight_norm = F.where(weight_norm == 0, F.ones_like(weight_norm), weight_norm)
 
 Review comment:
   Hi @ifeherva, thanks a lot for fixing the bug and I'm sorry for not handling this well. 
   
   I think the fix is good and it'll eliminate the nan problem. But it looks to me that the outcome is the same. The sum is 0 only when all weights in that row are 0. In that case, np.random.choice will still fail and go to the "except" clause. 
   
   To avoid confusion, I think it might be better to handle this explicitly instead of replying on np.random.choice to fail and go to the "except" clause (my bad :( ). What do you think? If it makes sense to you, will you be able to help improve this part? Thanks!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services