Skip to content

Conversation

@amyeroberts
Copy link
Contributor

@amyeroberts amyeroberts commented Apr 6, 2023

What does this PR do?

Abstracts out cropping logic to be a more generic crop function which other, more specific cropping functions e.g. center_crop can call.

Motivation:

  • The output of the CLIP feature extractor changed after Move Clip image utils to image_utils.py #17628. This was due to a difference in how the top and left coordinates were calculated resulting in some values being off by one.
  • The original CLIP feature extractor matched the original implementation
  • Having a more generic crop method enables each image processor to have its own center_crop logic with minimal code replication.

[BEFORE MERGING]: Verify this doesn't have large impact on any popular CLIP dependant pipelines

Fixes #22505

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

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@amyeroberts amyeroberts changed the title Add Crop Transformation [DO NOT MERGE] Add Crop Transformation Apr 6, 2023
@amyeroberts amyeroberts requested a review from ydshieh April 6, 2023 10:24
@amyeroberts amyeroberts marked this pull request as ready for review April 6, 2023 12:38
return new_image


def center_crop(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not able to comment on the exact line. But since center_crop is the one being overwritten in image_processing_clip.py, the checks like requires_backends(center_crop, ["vision"]) or if isinstance(image, PIL.Image.Image): etc. should be there too. Another approach is to make these kinds of input validations into the new crop, but I think it's not the most proper way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, the block

 if isinstance(image, PIL.Image.Image): warnings.warn( "PIL.Image.Image inputs are deprecated and will be removed in v4.26.0. Please use numpy arrays instead.", FutureWarning, ) image = to_numpy_array(image)

could be removed now I think, as we have

 if not isinstance(image, np.ndarray): raise ValueError(f"Input image must be of type np.ndarray, got {type(image)}")

But this could be done in a separate PR - as this is not the goal of the PR.

# | |||||| |
# | +----+ |
# | |
# *-----------*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love these pictures. But maybe mentioning what the 0 and 5 are. And which one is the image and another is the box.

Copy link
Collaborator

@ydshieh ydshieh 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 for the fix :-). Hope everything is as before except the bug being fixed.
A few nits for you to decide, but the one with the input validation should be addressed.

@huggingface huggingface deleted a comment from github-actions bot May 9, 2023
@huggingface huggingface deleted a comment from github-actions bot Jun 5, 2023
@ydshieh ydshieh added the WIP Label your PR/Issue with WIP for some long outstanding Issues/PRs that are work in progress label Jun 29, 2023
@huggingface huggingface deleted a comment from github-actions bot Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WIP Label your PR/Issue with WIP for some long outstanding Issues/PRs that are work in progress

3 participants