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

Fix post process function called in the instance segmentation example of mask2former #34588

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

OnTheThirdDay
Copy link
Contributor

@OnTheThirdDay OnTheThirdDay commented Nov 4, 2024

What does this PR do?

  • change doc of mask2former example of post_process_instance_segmentation to the right function call.
  • change doc of maskformer and mask2former on post_process_instance_segmentation for clarity on working with overlapping instances.
  • change examples in doc of segmentation models from image[::-1] to (image.height, image.width) for clarity.

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@Rocketknight1
Copy link
Member

This looks like a good fix to me, but cc @NielsRogge to confirm!

Copy link
Contributor

@NielsRogge NielsRogge 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! cc @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.

Thank you!

@@ -2416,7 +2416,7 @@ def forward(
>>> masks_queries_logits = outputs.masks_queries_logits

>>> # Perform post-processing to get instance segmentation map
>>> pred_instance_map = image_processor.post_process_semantic_segmentation(
>>> pred_instance_map = image_processor.post_process_instance_segmentation(
... outputs, target_sizes=[image.size[::-1]]
Copy link
Member

@qubvel qubvel Nov 4, 2024

Choose a reason for hiding this comment

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

It would be also nice to include:

Suggested change
... outputs, target_sizes=[image.size[::-1]]
... outputs, target_sizes=[(image.height, image.width)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we change this line in the examples for all three segmentation tasks?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sure!

@qubvel qubvel requested a review from ArthurZucker November 4, 2024 17:07
@OnTheThirdDay
Copy link
Contributor Author

OnTheThirdDay commented Nov 5, 2024

I just made another change to the doc of post_process_instance_segmentation of both maskformer and mask2former image processors.

I added this line, as current default behavior of the method will not deal with the cases where instances could overlap, but the descriptions related to return_coco_annotation and return_binary_maps, though clearly understandable, are not very intuitive for users to realize they need to use them.

If instances could overlap, set either return_coco_annotation or return_binary_maps to `True` to get the correct segmentation result.

Additionally, I changed this line to make it more precise for the description of the returned result covering the case where return_binary_maps is set to True.

`List[Dict]`: A list of dictionaries, one per image, each dictionary containing two keys:
            - **segmentation** -- A tensor of shape `(height, width)` where each pixel represents a `segment_id`, or
              `List[List]` run-length encoding (RLE) of the segmentation map if return_coco_annotation is set to
              `True`, or a tensor of shape `(num_instances, height, width)` if return_binary_maps is set to `True`.
              Set to `None` if no mask if found above `threshold`.

@OnTheThirdDay
Copy link
Contributor Author

the pr now has included above mentioned changes on doc.
@NielsRogge @qubvel @ArthurZucker

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

🤗 thanks

@ArthurZucker ArthurZucker merged commit 427b62e into huggingface:main Nov 19, 2024
7 checks passed
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
… of mask2former (huggingface#34588)

* Fix post process function called in the instance segmentation example of mask2former

* fix description and additional notes for post_process_instance_segmentation of maskformers

* remove white space in maskformers post_process_instance_segmentation doc

* change image.size[::-1] to height and width for clarity in segmentation examples
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
… of mask2former (huggingface#34588)

* Fix post process function called in the instance segmentation example of mask2former

* fix description and additional notes for post_process_instance_segmentation of maskformers

* remove white space in maskformers post_process_instance_segmentation doc

* change image.size[::-1] to height and width for clarity in segmentation examples
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.

5 participants