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

Rewrite draw_segmentation_masks and update gallery example to illustrate both instance and semantic segmentation models #3824

Merged
merged 23 commits into from
May 17, 2021

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented May 13, 2021

This PR changes the API of draw_segmentation_masks to something more generic that can support all models. See #3820 for more details.

It also significantly updates the gallery example with didactic instructions about the models outputs, and how to draw the masks with the new API.

@NicolasHug NicolasHug changed the title WIP Update visualization_utils example with new draw_segmentation_masks API Update visualization_utils example with new draw_segmentation_masks API May 13, 2021
@NicolasHug NicolasHug marked this pull request as ready for review May 13, 2021 14:34
@NicolasHug NicolasHug changed the title Update visualization_utils example with new draw_segmentation_masks API Rewrite draw_segmentation_masks and update gallery example to illustrate both instance and semantic segmentation models May 17, 2021
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

It looks great to me. Thanks @NicolasHug.

I know that you were not convinced about removing the float support but thank you for removing it from the PR to let more discussions around the issue.

This specific enhancement looks very nice and I think it's a great idea to expand the tool to support semantic segmentation. For me this is good to merge.

gallery/plot_visualization_utils.py Show resolved Hide resolved
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot @NicolasHug !

I have a minor comment, otherwise looks good to me.

sem_class_to_idx = {cls: idx for (idx, cls) in enumerate(sem_classes)}

# We normalize the masks of each image in the batch independently
normalized_masks = torch.stack([torch.nn.Softmax(dim=0)(masks) for masks in output])
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 instead do torch.nn.functional.softmax(masks, dim=0) or maybe masks.softmax(dim=0)?

Actually, you can probably just do

normalized_masks = output.softmax(dim=1)

no?

I think it's an anti-pattern to instantiate the nn.Module just to perform this operation once

Copy link
Member Author

@NicolasHug NicolasHug May 17, 2021

Choose a reason for hiding this comment

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

Ok, I'll use the functional API.

normalized_masks = output.softmax(dim=1) I think this would compute the softmax across the batch as well (dimension 0), and we wouldn't end up with values that sum to 1 for each image independently, the values would sum to 1 across the entire batch. That's actually something I wanted to double check with you, WDYT?

Edit: Hm they do seem to be the same. I'll use the simpler version as suggested then

Copy link
Member

Choose a reason for hiding this comment

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

I think it should do the expected thing if we use output.softmax(dim=1), but it's a good question.

We use it already in the loss for the segmentation models, which internally is handled by passing dim=1 to (log)softmax.

Internally, the softmax implementation splits the dimensions into those that are "batch dimensions" (which comes before the dim) and the "to be softmax" dimensions (which comes after `dim), see here for more details

@NicolasHug NicolasHug merged commit 5bb997c into pytorch:master May 17, 2021
@oke-aditya
Copy link
Contributor

oke-aditya commented May 17, 2021

So glad that the implementation is improved now! 😃 🎉

@NicolasHug
Copy link
Member Author

Thanks for your initial work @oke-aditya , it made this next version much easier to write and test!

facebook-github-bot pushed a commit that referenced this pull request May 19, 2021
…o illustrate both instance and semantic segmentation models (#3824)

Reviewed By: cpuhrsch

Differential Revision: D28538765

fbshipit-source-id: c8ff00c9e86f99f52e023a8b41c845ce51337671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants