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

Handle empty change indices in SAM's mask to rle conversion #35665

Merged
merged 16 commits into from
Jan 30, 2025

Conversation

MSt-10
Copy link
Contributor

@MSt-10 MSt-10 commented Jan 13, 2025

What does this PR do?

It handles empty change indices when the predicted mask is converted to rle.
Previously this caused an exception.

Fixes #35664

Who can review?

@ArthurZucker

@MSt-10 MSt-10 requested a review from qubvel as a code owner January 13, 2025 15:33
@qubvel
Copy link
Member

qubvel commented Jan 13, 2025

Hi @MSt-10! Thanks for the fix 🤗

Do you have a minimum reproducing example for the error? It would be also great to add a test

@MSt-10
Copy link
Contributor Author

MSt-10 commented Jan 13, 2025

Not really, it is highly dependent on the used image and the state of the fine-tuned model. I haven't observed the bug with the pre-trained model only after fine-tuning it.
(I have a fine-tuned model and an image that produces the error, but I don't think this is a practical test...)

@MSt-10
Copy link
Contributor Author

MSt-10 commented Jan 13, 2025

One could call the method directly with an empty mask. This should also produce the error I think.

@qubvel
Copy link
Member

qubvel commented Jan 13, 2025

One could call the method directly with an empty mask. This should also produce the error I think.

Such an example would be fine!

@MSt-10
Copy link
Contributor Author

MSt-10 commented Jan 13, 2025

Where should I add this test? In this file?

@qubvel
Copy link
Member

qubvel commented Jan 13, 2025

Yes!

@MSt-10
Copy link
Contributor Author

MSt-10 commented Jan 15, 2025

Are my tests sufficient? @qubvel

Copy link
Member

@qubvel qubvel 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 adding tests, its great to see you added for all 3 cases 👍 It can be merged in a single method IMO, smth like test_rle_encoding

Comment on lines 326 to 327
input_mask = torch.tensor([[[0, 1], [1, 1]]], dtype=torch.long)
rle = _mask_to_rle_tf(input_mask)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably pass TF tensor instead of torch?

@qubvel
Copy link
Member

qubvel commented Jan 30, 2025

Hey @MSt-10, hope you are doing well! Let me know if any help is needed from our side to finish this PR 🤗

@MSt-10
Copy link
Contributor Author

MSt-10 commented Jan 30, 2025

@qubvel Sorry for the delay but I don't use TF, hence I overlooked it. I hope it's correct now🤗

Copy link
Member

@qubvel qubvel 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 update, a nit comment

tests/models/sam/test_processor_sam.py Outdated Show resolved Hide resolved
Copy link
Member

@qubvel qubvel left a comment

Choose a reason for hiding this comment

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

Thanks!

@qubvel qubvel merged commit e4227eb into huggingface:main Jan 30, 2025
9 checks passed
Comment on lines +1379 to +1382
if input_mask[i, 0] == 0:
out.append({"size": [height, width], "counts": [height * width]})
else:
out.append({"size": [height, width], "counts": [0, height * width]})
Copy link
Collaborator

Choose a reason for hiding this comment

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

the only thing that changes here is the counts, we could reduce code with just updating counts 😉 but it's a nit!

yaswanth19 pushed a commit to yaswanth19/transformers that referenced this pull request Feb 5, 2025
…ace#35665)

* Handle empty change indices in RLE conversion for masks

* [test] Add unit tests for RLE encoding of masks in SamProcessor

* [test] Update RLE conversion tests to use TensorFlow implementation

* [test] Fix formatting in SamProcessorTest according to check_code_quality action

* [test] Fix formatting in SamProcessorTest according to check_code_quality

* [test] Refactored rle test cases into one test and used tf tensors in tf test cases

* [test] Fix: removed self parameter from refactored methods

* [test] Removed nested methods in run-length encoding tests for PyTorch and TensorFlow

* [test] Added description to individual to run-length encoding tests for PyTorch and TensorFlow.
bursteratom pushed a commit to bursteratom/transformers that referenced this pull request Feb 5, 2025
…ace#35665)

* Handle empty change indices in RLE conversion for masks

* [test] Add unit tests for RLE encoding of masks in SamProcessor

* [test] Update RLE conversion tests to use TensorFlow implementation

* [test] Fix formatting in SamProcessorTest according to check_code_quality action

* [test] Fix formatting in SamProcessorTest according to check_code_quality

* [test] Refactored rle test cases into one test and used tf tensors in tf test cases

* [test] Fix: removed self parameter from refactored methods

* [test] Removed nested methods in run-length encoding tests for PyTorch and TensorFlow

* [test] Added description to individual to run-length encoding tests for PyTorch and TensorFlow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RLE of SAM can't handle masks with no change
3 participants