mmf icon indicating copy to clipboard operation
mmf copied to clipboard

[feature] Add __eq__ method to SampleList

Open apsdehal opened this issue 4 years ago • 12 comments

🚀 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 avatar Aug 05 '20 17:08 apsdehal

Can I work on this?

stmugisha avatar Sep 11 '20 08:09 stmugisha

@steph-en-m Yes, go ahead.

apsdehal avatar Sep 11 '20 21:09 apsdehal

@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?

Shubham-Sahoo avatar Aug 21 '21 17:08 Shubham-Sahoo

@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.

apsdehal avatar Aug 23 '21 06:08 apsdehal

@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.

Shubham-Sahoo avatar Aug 23 '21 07:08 Shubham-Sahoo

@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.

apsdehal avatar Aug 23 '21 07:08 apsdehal

Hello @apsdehal Is this issue closed?

AditiThirdEye avatar Aug 24 '21 21:08 AditiThirdEye

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

Shubham-Sahoo avatar Aug 25 '21 12:08 Shubham-Sahoo

@AditiThirdEye @Shubham-Sahoo is working on this.

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

apsdehal avatar Aug 27 '21 15:08 apsdehal

@apsdehal yeah I'm working on the comments.

Shubham-Sahoo avatar Aug 27 '21 17:08 Shubham-Sahoo

@apsdehal I have pushed the changes.

Shubham-Sahoo avatar Sep 06 '21 05:09 Shubham-Sahoo

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

vaish-muk avatar Jan 27 '22 08:01 vaish-muk