davidelct commented 2 years ago

Thanks for working on EGG and opening a PR! Please fill this template to help us understand what are you introducing in EGG.

Description

Added an __add__ method to the Interaction dataclass
Changed the return statement in dump_interactions function to return the full interaction
Renamed continous_communication.py to continuous_communication.py and all the related imports accordingly

Related Issue (if any)

The interactions bug could be reproduced by calling the dump_interactions function, which would fail raising the TypeError exception: unsupported operand type(s) for +: 'Interaction' and 'Interaction'.

Motivation and Context

The Interaction dataclass lacked an __add__ method to control the behavior of the + operator between two interaction objects: this would prevent dump_interactions from accumulating the interaction objects in the full_interaction variable. Moreover, the dump_interactions function returned the interaction variable instead of the full_interaction. (https://github.com/facebookresearch/EGG/issues/240#issue-1150682221) The continuous_communication.py file name had a typo. (https://github.com/facebookresearch/EGG/issues/239#issue-1150680172)

How Has This Been Tested?

Tested by adding two Interaction objects into a single Interaction, and checked that no data is missing. Tested dump_interactions behavior by checking that it returns the full interaction. Pytest from root dir passes all tests.

facebook-github-bot commented 2 years ago

Hi @davidelct!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, 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.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

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

facebook-github-bot commented 2 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

robertodessi commented 2 years ago

Hi @davidelct, thanks for opening the PR! It looks great! I left a couple of comments, I think that after integrating those it's good to go! Thanks!

davidelct commented 1 year ago

hi @robertodessi ! thanks for your suggestions. the following changes have been made to __add__:

doctesting has been included
aux and aux_info are now added correctly, without overwriting of shared keys
_check_cat is now used instead of simply using torch.cat()

let me know if there is anything else we could add here and thanks for your help !

robertodessi commented 1 year ago

Hi @davidelct,

Thanks a lot for fixing this! It looked already great to me, but I left a minor minor comment (not sure if it's a typo in the docstring) and two suggestions. What do you think?

davidelct commented 1 year ago

Hi @robertodessi, thanks a lot for the suggestions ! They look good to me, I incorporated as follows:

in __add__ the aux dicts are now combined with a single function, so as to avoid repetitions in code and to make it consistent with how the rest of the data is combined too
yes, there was a typo in the docstrings for dump_interactions: now removed
minor: i made the doctest for __add__ a bit more readable

thanks !