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.

Sign up for GitHub

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

Jump to bottom

Add ViTMatte #25843

Merged
merged 22 commits into from
Sep 19, 2023
Merged

Add ViTMatte #25843

merged 22 commits into from
Sep 19, 2023

Conversation

NielsRogge
Copy link
Contributor

@NielsRogge NielsRogge commented Aug 29, 2023 •
edited
Loading

What does this PR do?

This PR supersedes #25051 and #25524.

To do:

  • simplify image processor
  • fix image processor tests => @amyeroberts should I overwrite the following tests, since the image processor also requires trimaps to be passed?
FAILED tests/models/vitmatte/test_image_processing_vitmatte.py::VitMatteImageProcessingTest::test_call_numpy - TypeError: preprocess() missing 1 required positional argument: 'trimaps'
FAILED tests/models/vitmatte/test_image_processing_vitmatte.py::VitMatteImageProcessingTest::test_call_numpy_4_channels - TypeError: preprocess() missing 1 required positional argument: 'trimaps'
FAILED tests/models/vitmatte/test_image_processing_vitmatte.py::VitMatteImageProcessingTest::test_call_pil - TypeError: preprocess() missing 1 required positional argument: 'trimaps'
FAILED tests/models/vitmatte/test_image_processing_vitmatte.py::VitMatteImageProcessingTest::test_call_pytorch - TypeError: preprocess() missing 1 required positional argument: 'trimaps'
FAILED tests/models/vitmatte/test_image_processing_vitmatte.py::VitMatteImageProcessingTest::test_image_processor_from_dict_with_kwargs - AssertionError: 42 != {'height': 42, 'width': 42}

amyeroberts
amyeroberts reviewed Sep 6, 2023
View reviewed changes
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 adding this model!

I've done an initial pass. Mostly looks good, but there's a few places which need to be updated before it's transformers ready. Once those comments have been resolved and the tests are all passing ping me again for another review :)

cc @rafaelpadilla

docs/source/en/model_doc/vitmatte.md Show resolved Hide resolved
src/transformers/models/vitmatte/configuration_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/configuration_vitmatte.py Outdated Show resolved Hide resolved
tests/models/vitmatte/test_modeling_vitmatte.py Outdated Show resolved Hide resolved
tests/models/vitmatte/test_modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 7, 2023 •
edited
Loading

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

@NielsRogge
Copy link
Contributor Author

@amyeroberts thanks for your review, I've addressed all comments. However regarding renaming the outputs from alphas to logits => are we sure we want to do this, given that image matting models output the alpha values, not really logits?

amyeroberts
amyeroberts reviewed Sep 13, 2023
View reviewed changes
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 iterating!

Just a few small changes and I think we're good to merge. It will be great to have this model added!

Re alphas vs logits in the model output. Happy for it to be alphas as we apply a sigmoid on the output so it's [0, 1]

src/transformers/models/vitmatte/image_processing_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/image_processing_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/configuration_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
@amyeroberts
Copy link
Collaborator

cc @rafaelpadilla for reference

@amyeroberts amyeroberts mentioned this pull request Sep 14, 2023
Merged
4 tasks
@NielsRogge
Copy link
Contributor Author

Addressed all comments. Feel free to merge if approved

amyeroberts
amyeroberts approved these changes Sep 14, 2023
View reviewed changes
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 adding this model!

Just a tiny tiny nit. @rafaelpadilla could you handle merging this PR once resolved?

src/transformers/models/vitmatte/modeling_vitmatte.py Outdated Show resolved Hide resolved
@NielsRogge
Copy link
Contributor Author

Failing tests are flaky/unrelated

rafaelpadilla
rafaelpadilla reviewed Sep 18, 2023
View reviewed changes
Copy link
Contributor

@rafaelpadilla rafaelpadilla 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 to me! :)
Excellent work, @NielsRogge

@rafaelpadilla
Copy link
Contributor

@NielsRogge
It seems to me that every comment was addressed and now all tests are passing.
I believe now it is ready to merge. May I?

@NielsRogge
Copy link
Contributor Author

Yes

@rafaelpadilla rafaelpadilla merged commit 7d6354e into huggingface:main Sep 19, 2023
23 checks passed
MKhalusova pushed a commit to MKhalusova/transformers that referenced this pull request Sep 19, 2023
@NielsRogge @MKhalusova
Add ViTMatte (huggingface#25843)
31e6a38 
* First draft

* Simplify image processor

* Fix rebase

* Address comments

* Address more comments

* Address more comments

* Address more comments

* Address more comments

* Improve pad_image

* Add tests

* Update integration test

* Fix image processor tests

* Fix model tests

* Convert checkpoints

* Fix doc tests

* Remove file

* Apply suggestions

* Address comments

* Fix typing hint

* Add batch_norm_eps

* Address comments

* Fix style
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
@NielsRogge @parambharat
Add ViTMatte (huggingface#25843)
fb28631 
* First draft

* Simplify image processor

* Fix rebase

* Address comments

* Address more comments

* Address more comments

* Address more comments

* Address more comments

* Improve pad_image

* Add tests

* Update integration test

* Fix image processor tests

* Fix model tests

* Convert checkpoints

* Fix doc tests

* Remove file

* Apply suggestions

* Address comments

* Fix typing hint

* Add batch_norm_eps

* Address comments

* Fix style
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
@NielsRogge @blbadger
Add ViTMatte (huggingface#25843)
019b213 
* First draft

* Simplify image processor

* Fix rebase

* Address comments

* Address more comments

* Address more comments

* Address more comments

* Address more comments

* Improve pad_image

* Add tests

* Update integration test

* Fix image processor tests

* Fix model tests

* Convert checkpoints

* Fix doc tests

* Remove file

* Apply suggestions

* Address comments

* Fix typing hint

* Add batch_norm_eps

* Address comments

* Fix style
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
@NielsRogge @EduardoPach
Add ViTMatte (huggingface#25843)
e971d2d 
* First draft

* Simplify image processor

* Fix rebase

* Address comments

* Address more comments

* Address more comments

* Address more comments

* Address more comments

* Improve pad_image

* Add tests

* Update integration test

* Fix image processor tests

* Fix model tests

* Convert checkpoints

* Fix doc tests

* Remove file

* Apply suggestions

* Address comments

* Fix typing hint

* Add batch_norm_eps

* Address comments

* Fix style
@NielsRogge NielsRogge mentioned this pull request Apr 12, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelpadilla rafaelpadilla rafaelpadilla left review comments

@amyeroberts amyeroberts amyeroberts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NielsRogge @HuggingFaceDocBuilderDev @amyeroberts @rafaelpadilla