Skip to content

Fix ViTForMaskedImageModeling doc example#22186

Closed
ydshieh wants to merge 2 commits into
mainfrom
fix_doc
Closed

Fix ViTForMaskedImageModeling doc example#22186
ydshieh wants to merge 2 commits into
mainfrom
fix_doc

Conversation

@ydshieh
Copy link
Copy Markdown
Collaborator

@ydshieh ydshieh commented Mar 15, 2023

What does this PR do?

Same #22185

@ydshieh ydshieh changed the title Fix ViTForMaskedImageModeling example in documentation Fix ViTForMaskedImageModeling doc example Mar 15, 2023
@amyeroberts
Copy link
Copy Markdown
Contributor

amyeroberts commented Mar 15, 2023

@ydshieh @alaradirik @fxmarty The issue coming from #22152 was an oversight on my part about breaking changes. Perhaps we should revert that PR first and then agree how to introduce this change as it is intended to be added to other vision model?

@ydshieh
Copy link
Copy Markdown
Collaborator Author

ydshieh commented Mar 15, 2023

Oh, I thought it was a new model head! Indeed a breaking change there. Good for me to revert that PR, but would be nice to talk to Sylvain or Lysandre first (if you feel necessary). I will leave you judge.

Regarding a solution if we really want to have this new attribute and the new name MaskedImageCompletionOutput, adding a new property (named logits) to MaskedImageCompletionOutput might be a way, but I didn't think about this deeply.

@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

HuggingFaceDocBuilderDev commented Mar 15, 2023

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

@ydshieh ydshieh marked this pull request as draft March 15, 2023 16:46
@ydshieh
Copy link
Copy Markdown
Collaborator Author

ydshieh commented Mar 15, 2023

Converted to draft for now

@amyeroberts
Copy link
Copy Markdown
Contributor

@ydshieh Yep - let's get @LysandreJik and @sgugger 's opinions.

I think having the logits param is probably the best solution. As far as I know, it's very rare to check against the model output type itself. I believe reconstruction was chosen because of the ImageSuperResolutionOutput data class. As mentioned in the original PR - we probably do want a different model type to be returned as the documented shapes are incorrect.

@alaradirik - could you open a PR to revert the changes?

@amyeroberts
Copy link
Copy Markdown
Contributor

Actually, it's late for @alaradirik. I'll open the PR now.

@sgugger
Copy link
Copy Markdown
Collaborator

sgugger commented Mar 15, 2023

Yes we can't rename the parameter in the outputs like that for a model that has been around for a bit. What is even more annoying is that the commit was in the release, so we will need to make a patch with the fix.

@ydshieh
Copy link
Copy Markdown
Collaborator Author

ydshieh commented Mar 15, 2023

Close this PR as it's clear we will and have to definitely use the original logits.

@ydshieh ydshieh closed this Mar 15, 2023
@alaradirik
Copy link
Copy Markdown
Contributor

alaradirik commented Mar 16, 2023

Sorry for being late to comment, I added the MaskedImageCompletionOutput to replace the inaccurate MaskedLMOutput class used by the masked image modeling heads (ViT and DeiT). Neither of these models have any checkpoints on the hub as mentioned in #22152 . Swin's MIM head has its own output class but no fine-tuned checkpoints for the MIM task either.

With that said, ViT and Swin's MIM heads are implementations of SimMIM and SimMIM have recently released fine-tuned checkpoints for these two models (as opposed to the base model weights on the hub for Swin MIM head). I'm planning to convert these checkpoints and add a masked-image-completion pipeline after @sheonhan merges the ICT PR (a contemporary, better performing MIM model). It'd be great to add an output class and fix inaccurate class output (listed as a language model in the docs) before that. While logits is not an accurate output name in this case as the model returns full reconstructed images, I could replace reconstruction with logits and open a new PR.

What do you think @amyeroberts @sgugger?
CC @LysandreJik @ydshieh

@ydshieh ydshieh deleted the fix_doc branch March 17, 2023 12:58
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