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

fixed Mask2Former image processor segmentation maps handling #33364

Conversation

maciej-adamiak
Copy link
Contributor

Fixes segmentation maps padding size calculation when handling multichannel images using previously inferred input_data_format.

Fixes #33295

Before submitting

Who can review?

Anyone

@maciej-adamiak maciej-adamiak force-pushed the mask2former-image-processor-handle-multichannel-images branch from 5c73b13 to 7aad102 Compare September 6, 2024 22:38
@LysandreJik
Copy link
Member

cc @amyeroberts @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 a lot for working on this and adding tests, just a nit comment

Comment on lines 54 to 55
image_mean=0.5,
image_std=0.5,
Copy link
Member

Choose a reason for hiding this comment

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

Please, revert to the original state :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a revert is applied both values will become 3-element lists and will not be compatible with tests run on images with a different number of channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to be super precise I can write '[0.5] * num_channels'.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that way will be better, just to be sure normalization work for this case 👍

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 the update, added a few comments regarding tests

Comment on lines 276 to 280
with pytest.raises(ValueError, match="Unable to infer channel dimension format"):
common(num_channels=5, numpify=True, do_resize=False)

with pytest.raises(TypeError, match=r"Cannot handle this data type: .*"):
common(num_channels=5, numpify=True, input_data_format=ChannelDimension.LAST)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, missed this for the first time, we use unittest for testing, can you please update to with self.assertRaises(ValueError) or assertRaisesRegex and remove import pytest on the top of the module.

Comment on lines 245 to 248
self.image_processor_tester.num_channels = num_channels
self.image_processor_tester.do_resize = do_resize
self.image_processor_tester.image_mean = [0.5] * num_channels
self.image_processor_tester.image_std = [0.5] * num_channels
Copy link
Member

Choose a reason for hiding this comment

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

not sure it's a good way to modify class properties in place, that may lead to some hard-to-debug issues later. You can recreate an instance here with an updated image processor dict.

Comment on lines 272 to 274
common(num_channels=1, numpify=True)
common(num_channels=2, numpify=True, input_data_format=ChannelDimension.LAST)
common(num_channels=5, numpify=True, input_data_format=ChannelDimension.LAST, do_resize=False)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please make a call with explicit input_data_format=ChannelDimension.FIRST for better readability of the test?

Copy link
Collaborator

@amyeroberts amyeroberts 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 fixing! With @qubvel's suggestions all looks good to me -- let's get his final approval and then we can merge :)

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 the update!

@qubvel qubvel merged commit 8e8e7d8 into huggingface:main Sep 10, 2024
14 checks passed
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
…face#33364)

* fixed mask2former image processor segmentation maps handling

* introduced review suggestions

* introduced review suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mask2FormerImageProcessor - fails to process multichannel image
4 participants