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

[feature] Add __eq__ method to SampleList #454

Closed
apsdehal opened this issue Aug 5, 2020 · 12 comments
Closed

[feature] Add __eq__ method to SampleList #454

apsdehal opened this issue Aug 5, 2020 · 12 comments
Labels
bootcamp For FB newcomers enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@apsdehal
Copy link
Contributor

apsdehal commented Aug 5, 2020

🚀 Feature

SampleList frequently require comparisons in tests. It would good to add __eq__ method to SampleList

Pitch

  • Compare list of keys and fail early
  • Compare each item
  • Take special care of tensors
  • Add tests

Note: If you are a bootcamper working on this task, assign the internal task to yourself: T71222206

@apsdehal apsdehal added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers bootcamp For FB newcomers labels Aug 5, 2020
@stmugisha
Copy link
Contributor

Can I work on this?

@apsdehal
Copy link
Contributor Author

@steph-en-m Yes, go ahead.

@Shubham-Sahoo
Copy link

@apsdehal I would like to work on this issue. I'm not clear of what to compare inside the SampleList __eq__ method. Can you give a clarification on this?

@apsdehal
Copy link
Contributor Author

@Shubham-Sahoo Hi, thanks for your interest. In general, iterate over the keys, check if the type is tensor and compare them using torch.equal otherwise, normally compare them as it is. Let me know if you got the idea.

@Shubham-Sahoo
Copy link

@apsdehal So it is like comparing all the samples in the sampleList with a given tensor. So the __eq__ method will take an input tensor as well. Please correct me if I'm wrong.

@apsdehal
Copy link
Contributor Author

@Shubham-Sahoo To understand the purpose of __eq__ check https://www.pythontutorial.net/python-oop/python-__eq__/. I hope this makes it clear. In case of SampleList __eq__ method will take in another sample list with which it is being compared to be equal or not.

msethi006 added a commit to msethi006/mmf that referenced this issue Aug 24, 2021
Added __eq__ method as requested facebookresearch#454
@AditiThirdEye
Copy link

Hello @apsdehal
Is this issue closed?

@Shubham-Sahoo
Copy link

@apsdehal Can you review my PR and suggest if any changes are required.

@apsdehal
Copy link
Contributor Author

@AditiThirdEye @Shubham-Sahoo is working on this.

@Shubham-Sahoo I reviewed your PR, take a look at my comments.

@Shubham-Sahoo
Copy link

@apsdehal yeah I'm working on the comments.

@Shubham-Sahoo
Copy link

@apsdehal I have pushed the changes.

@vaish-muk
Copy link

Hi @apsdehal is the issue still open? can I take it up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bootcamp For FB newcomers enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants