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

[SegFormer] Add support for segmentation masks with one label #20279

Merged
merged 4 commits into from Dec 20, 2022

Conversation

NielsRogge
Copy link
Contributor

What does this PR do?

This PR makes it possible to fine-tune SegFormer in case you have a mask containing only a single value, i.e. your mask could look like [[255, 0], [0, 255]]. In this case, config.num_labels = 1 and the ignore index is 255.

If this works fine, then we can add it to any other model supported by AutoModelForSemanticSegmentation.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 16, 2022

The documentation is not available anymore as the PR was closed or merged.

@dimkoss11
Copy link

dimkoss11 commented Nov 25, 2022

How soon will this PR be done? And why using BCEWithLogitsLoss not Dice loss?

@jamilkarabaev
Copy link

when is this going to be closed. Our team would like to use it

@NielsRogge NielsRogge marked this pull request as ready for review December 16, 2022 10:33
@jamilkarabaev
Copy link

when this becomes active, how would one use it.

thank you nielsrogge for sending for review.

@jamilkarabaev
Copy link

hello @NielsRogge

hope I didnt disturb

I tried to peel off the classifier in pytorch and change the output channels to one, then manually compute the loss instead of getting it from the segformer huggingface object, so basically I just got the output and then did a dice loss myself.

So i tried to write bin seg myself kinda, and I started to get a bunch of negative values.

Any idea why that happened, and how I could fix it? I mean, I replaced the last conv2d layer.

Copy link
Contributor

@alaradirik alaradirik left a comment

Choose a reason for hiding this comment

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

Looks good to me! But I agree with the commenter about using Dice loss instead of BCEWithLogitsLoss.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Looks good to me once the loss comment is resolved.

@NielsRogge NielsRogge merged commit 2875fa9 into huggingface:main Dec 20, 2022
MKhalusova pushed a commit to MKhalusova/transformers that referenced this pull request Dec 28, 2022
…gface#20279)

* Add support for binary segmentation

* Fix loss calculation and add test

* Remove space

* use fstring

Co-authored-by: Niels Rogge <nielsrogge@Nielss-MacBook-Pro.local>
Co-authored-by: Niels Rogge <nielsrogge@Nielss-MBP.localdomain>
amyeroberts pushed a commit to amyeroberts/transformers that referenced this pull request Jan 4, 2023
…gface#20279)

* Add support for binary segmentation

* Fix loss calculation and add test

* Remove space

* use fstring

Co-authored-by: Niels Rogge <nielsrogge@Nielss-MacBook-Pro.local>
Co-authored-by: Niels Rogge <nielsrogge@Nielss-MBP.localdomain>
silverriver pushed a commit to silverriver/transformers that referenced this pull request Jan 6, 2023
…gface#20279)

* Add support for binary segmentation

* Fix loss calculation and add test

* Remove space

* use fstring

Co-authored-by: Niels Rogge <nielsrogge@Nielss-MacBook-Pro.local>
Co-authored-by: Niels Rogge <nielsrogge@Nielss-MBP.localdomain>
venkat-natchi pushed a commit to venkat-natchi/transformers that referenced this pull request Jan 22, 2023
…gface#20279)

* Add support for binary segmentation

* Fix loss calculation and add test

* Remove space

* use fstring

Co-authored-by: Niels Rogge <nielsrogge@Nielss-MacBook-Pro.local>
Co-authored-by: Niels Rogge <nielsrogge@Nielss-MBP.localdomain>
miyu386 pushed a commit to miyu386/transformers that referenced this pull request Feb 9, 2023
…gface#20279)

* Add support for binary segmentation

* Fix loss calculation and add test

* Remove space

* use fstring

Co-authored-by: Niels Rogge <nielsrogge@Nielss-MacBook-Pro.local>
Co-authored-by: Niels Rogge <nielsrogge@Nielss-MBP.localdomain>
@aegonwolf
Copy link

Is this now automatically using dice loss when we set num_labels = 1? Maybe I missed it but it seems the documentation doesn't explain it.

@NielsRogge
Copy link
Contributor Author

HHi @aegonwolf,

When config.num_labels = 1, the binary cross-entropy loss is used, as can be seen here:

elif self.config.num_labels == 1:
valid_mask = ((labels >= 0) & (labels != self.config.semantic_loss_ignore_index)).float()
loss_fct = BCEWithLogitsLoss(reduction="none")
loss = loss_fct(upsampled_logits.squeeze(1), labels.float())
loss = (loss * valid_mask).mean()
.

@nikolaJovisic
Copy link

What about a sigmoid function at the end or any way of scaling outputs to [0, 1] range?

@NielsRogge
Copy link
Contributor Author

@nikolaJovisic the BCE loss uses sigmoid.

@hungphongtran-pixta
Copy link

@NielsRogge Thank you for your amazing work. However I am still struggling with this task, can you make an example notebook on how to finetune Segformer for 1 class only please? Thank you a lot!

@nikolaJovisic
Copy link

@NielsRogge modeling_segformer.py script is changed, but modeling_tf_segformer.py seems to be unchanged, I get an error there when I try to train model for 1 class only.

@pratikkorat26
Copy link

Any update on this task ? Any example notebook ?

@nikolaJovisic
Copy link

Any update on this task ? Any example notebook ?

Hi,

Back when I was working on this, the BinaryCrossentropy variant was merged and the issue was closed, but if I remember correctly it was done only for PyTorch version, so I implemented the Tensorflow version myself on a fork of this repository, you can find it on my profile, never made the effort to open a PR tho. Hope this might help. The fine-tuning itself was straightforward after that if I remember correctly.

Best wishes,
Nikola

@NielsRogge
Copy link
Contributor Author

@nikolaJovisic would be great if you could open a PR for it!

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.

None yet

10 participants