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

Revert to rescale and safely handle flag in owlvit config #18750

Merged

Conversation

amyeroberts
Copy link
Collaborator

What does this PR do?

Reverts the rescale_image method name to back to rescale. This keeps the method inline with other method names such as normalize.

This undoes the renaming in #18677 done to address failing tests caused by rescale key being in OWL-ViT config and the introduction of a rescale method in #18499.

The OWL-ViT config has since been updated. As the model has been released, we rename the key in the config in case rescale is there for backwards compatibility.

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?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 24, 2022

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

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.

Thank you! And sorry about the double PR to revert the changes

Comment on lines +91 to +94
if "rescale" in kwargs:
rescale_val = kwargs.pop("rescale")
kwargs["do_rescale"] = rescale_val

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! Will we eventually remove this part?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I think we can leave it for a short period of time so that if anyone pushes to the hub and updates the model, pushing the feature extractor with it, then the rescale parameter will be renamed in the preprocessor_config.json. After that, we could add a warning if rescale is in detected in the config - asking the person to update their config as it will no longer be supported as a flag in the future.

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!

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks for handling the backwards compatibility @amyeroberts

@LysandreJik LysandreJik merged commit 8c4a114 into huggingface:main Aug 29, 2022
oneraghavan pushed a commit to oneraghavan/transformers that referenced this pull request Sep 26, 2022
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