`tf.sign(tf.abs(tf.reduce_sum` vs `tf.sign(tf.reduce_sum(tf.abs(` for generating masks? about sasrec HOT 4 CLOSED

Thanks for checking this!

from sasrec.

Thanks for looking into the details of the implementation. It seems the self-attention modules in the original repo (https://github.com/Kyubyong/transformer) is updated, and the code is looking mucher simpler and cleaner right now. And I remember that I've used that for SASRec in my own experiments (e.g. replacing the code in modules.py), and it seems work as well. I'm not sure if they fixed this issue. If that's the case, we may update the github code to use the newer implementation of self-attention.

from sasrec.

Thanks for looking into the details of the implementation. It seems the self-attention modules in the original repo (https://github.com/Kyubyong/transformer) is updated, and the code is looking mucher simpler and cleaner right now. And I remember that I've used that for SASRec in my own experiments (e.g. replacing the code in modules.py), and it seems work as well. I'm not sure if they fixed this issue. If that's the case, we may update the github code to use the newer implementation of self-attention.

Thank you Wang-Cheng! It's just a small issue, and yes, they have updated the transformer repo and commented this part(as they have identified our life would be easier if masks are set as arguments and easily obtained in seq level rather than in seq embedding level by src_masks = tf.math.equal(x, 0) lol), I'll try to get it confirmed by cc the thread to transformer repo you mentioned and keep you updated for future feedback.

The issue got created just FYI, no need to update the repo for experiment usage, but it would be better to let peers know the detail.

Have a good day :)

from sasrec.

Thanks for looking into the details of the implementation. It seems the self-attention modules in the original repo (https://github.com/Kyubyong/transformer) is updated, and the code is looking mucher simpler and cleaner right now. And I remember that I've used that for SASRec in my own experiments (e.g. replacing the code in modules.py), and it seems work as well. I'm not sure if they fixed this issue. If that's the case, we may update the github code to use the newer implementation of self-attention.

Oh, I just found they did correct it before commenting the part, pls see:

https://github.com/Kyubyong/transformer/blob/fb023bb097e08d53baf25b46a9da490beba51a21/modules.py#L134

as I expected, abs should be applied before reduce_sum, no need to ping the guys in that repo now :)

from sasrec.