Skip to content

Add default_to_square_for_size to CLIPImageProcessor#26965

Merged
ydshieh merged 5 commits into
mainfrom
change_clip
Oct 24, 2023
Merged

Add default_to_square_for_size to CLIPImageProcessor#26965
ydshieh merged 5 commits into
mainfrom
change_clip

Conversation

@ydshieh

@ydshieh ydshieh commented Oct 20, 2023

Copy link
Copy Markdown
Collaborator

What does this PR do?

Add default_to_square_for_size to CLIPImageProcessor.

As in the file, we also have crop_size = get_size_dict(crop_size, default_to_square=True, param_name="crop_size"), so I give the name default_to_square_for_size for get_size_dict(size, default_to_square=default_to_square_for_size) to avoid confusion.

Otherwise, I can give it default_to_square, and we may add default_to_square_for_crop_size later (or in this PR).

@ydshieh ydshieh requested a review from ArthurZucker October 20, 2023 16:23

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few very small nits

image_mean: Optional[Union[float, List[float]]] = None,
image_std: Optional[Union[float, List[float]]] = None,
do_convert_rgb: bool = True,
default_to_square_for_size: bool = False,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
default_to_square_for_size: bool = False,
use_square_size: bool = False,

or something like this!

Comment on lines +87 to +88
default_to_square_for_size (`bool`, *optional*, defaults to `False`):
The value to be passed to `get_size_dict` as `default_to_square` when computing the image size.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

let's say what it changes depending on this values!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

        use_square_size (`bool`, *optional*, defaults to `False`):
            The value to be passed to `get_size_dict` as `default_to_square` when computing the image size. If the
            `size` argument in `get_size_dict` is an `int`, it determines whether to default to a square image or not.
            Note that this attribute is not used in computing `crop_size` via calling `get_size_dict`.

Comment on lines +87 to +90
use_square_size (`bool`, *optional*, defaults to `False`):
The value to be passed to `get_size_dict` as `default_to_square` when computing the image size. If the
`size` argument in `get_size_dict` is an `int`, it determines whether to default to a square image or not.
Note that this attribute is not used in computing `crop_size` via calling `get_size_dict`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Okay good enough for me, would be better to know what size are used if false for example

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I take this from the original get_size_dict.

def get_size_dict(
        ...
        ...
        default_to_square (`bool`, *optional*, defaults to `True`):
            If `size` is an int, whether to default to a square image or not.

other places like def get_resize_output_image_size

has sth like

            How to convert `size` when it is a single int. If set to `True`, the `size` will be converted to a square
            (`size`,`size`). If set to `False`, will replicate
            [`torchvision.transforms.Resize`](https://pytorch.org/vision/stable/transforms.html#torchvision.transforms.Resize)
            with support for resizing only the smallest edge and providing an optional `max_size`.

but this method has more logic than the canonical get_size_dict

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Oct 20, 2023

Copy link
Copy Markdown

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

@ydshieh ydshieh requested a review from ArthurZucker October 20, 2023 17:06

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! I am pinging @amyeroberts so that she sees it when she comes back. Feel free to merge

@ydshieh

ydshieh commented Oct 20, 2023

Copy link
Copy Markdown
Collaborator Author

Thanks a lot !

@ydshieh

ydshieh commented Oct 20, 2023

Copy link
Copy Markdown
Collaborator Author

I forgot to change this line

        output_size = get_resize_output_image_size(
            image, size=size["shortest_edge"], default_to_square=False, input_data_format=input_data_format
        )

to

        output_size = get_resize_output_image_size(
            image, size=size["shortest_edge"], default_to_square=self.use_square_size, input_data_format=input_data_format
        )

will do it but wait on next Monday to merge

@amyeroberts amyeroberts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding!

@ydshieh ydshieh merged commit fc142bd into main Oct 24, 2023
@ydshieh ydshieh deleted the change_clip branch October 24, 2023 09:08
i4never pushed a commit to i4never/transformers that referenced this pull request Oct 25, 2023
…#26965)

* fix

* fix

* fix

* fix

* fix

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
…#26965)

* fix

* fix

* fix

* fix

* fix

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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.

4 participants