search

facebookresearch / EGG

EGG: Emergence of lanGuage in Games
MIT License
286 stars 99 forks source link

Fixed bug in calculating topological similarity #129

Closed Shawn-Guo-CN closed 3 years ago

Shawn-Guo-CN commented 3 years ago

Previous the maximum of the implemented edit distance is 0.25, which would makes the correlation between it with other measurements smaller than the true one.

To be specific, editdistance.eval(x, y) / (len(x) + len(y)) would be expected to 0.5, thus editdistance.eval(x, y) / (len(x) + len(y)) / 2 would be at most 0.25. I guess that the implementation was meant to be editdistance.eval(x, y) / ((len(x) + len(y)) / 2)

facebook-github-bot commented 3 years ago

Hi @Shawn-Guo-CN!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot commented 3 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

robertodessi commented 3 years ago

Hi @Shawn-Guo-CN , thanks for spotting this. I'm just not sure this capped the topographic similarity as the previous distance was just a different distance function as the one we expected, I believe topsim is always bounded to 1 and -1 as it's a correlation value.

Many thanks for the fix!

Shawn-Guo-CN commented 3 years ago

Hi @robertodessi,

Theoretically, the topo-sim should keep the same with the capped edit distance. But, according to my experiment experience, it seems that the distance has a big effect on the topo-sim value of emergeng language.

I'm not sure the final reason for that, still checking all the relevant modules. Will update you if I found it.

Best, Shawn