Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] '__eq__' method for SampleList #1053

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Shubham-Sahoo
Copy link

@Shubham-Sahoo Shubham-Sahoo commented Aug 23, 2021

Added the eq method and tests for SampleList facebookresearch#454

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 23, 2021
@Shubham-Sahoo Shubham-Sahoo changed the title __eq__ method for SampleList [feat] '__eq__' method for SampleList Aug 25, 2021
Copy link
Contributor

@apsdehal apsdehal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing to MMF! Awesome first PR. I have left some comments. Address those and it should be good to land.

b = set(fields_other)

# Comparison between keys and early fail
if a==b:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter won't be happy with this.


# Compare Lists
else:
if not self[field]==other.get_field(field):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use either key access on both sides or use get_field on both sides.

self.assertFalse(sample_list1 == sample_list3)
self.assertFalse(sample_list1 == sample_list4)
self.assertFalse(sample_list2 == sample_list4)
self.assertFalse(sample_list1 == sample_list5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert more complex cases and cases which are not equal as well. Try testing SampleList inside SampleList or dict inside SampleList.

@Shubham-Sahoo
Copy link
Author

Shubham-Sahoo commented Aug 28, 2021

@apsdehal I have pushed the required changes, please have a look.

@Shubham-Sahoo Shubham-Sahoo requested a review from apsdehal August 28, 2021 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants