fixing + testing center_crop, so it can accept odd sizes
Author: rafaelpadilla
Pull request: yes

# What does this PR do?

Fixes #22505 

Our `center_crop` function does not give proper results if `orig_height - crop_height` is odd or if `orig_width - crop_width` is odd. 

It seems that our code [here](https://github.com/huggingface/transformers/blob/main/src/transformers/image_transforms.py#L461C66-L461C88) and [here](https://github.com/huggingface/transformers/blob/main/src/transformers/image_transforms.py#L464) alerts about this problem.

This problem explains why:
* Our `test_center_crop` fails [here](https://github.com/huggingface/transformers/blob/e469be340673d1f6931eb22562efd2be7f5a5b8d/tests/test_image_transforms.py#L329) if we try image sizes with odd height or width. e.g. changing this line [here](https://github.com/huggingface/transformers/blob/e469be340673d1f6931eb22562efd2be7f5a5b8d/tests/test_image_transforms.py#L317) to `image = np.random.randint(0, 256, (3, 223, 223))` will make the test fail.
* Our results are different than OpenAI image features, as reported in issue #22505 

This PR fixes this problem and includes pytests comparing the output of our `center_crop` function to `torchvision.transforms.CenterCrop` with different image sizes and expected output sizes.

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? 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](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [x] Did you write any new necessary tests?


## Who can review?
@ArthurZucker and @amyeroberts


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_26238). All of your documentation changes will be reflected on that endpoint.

> Thanks for fixing!
> 
> This is likely going to result in changes in outputs for some users, in particular for CLIP which is our most used vision model. As it's fixing logic I think this OK but we should be aware of this for any future issues. @rafaelpadilla could you run the slow model tests for CLIP and any other model which uses center_crop in its image processor before merging?

I identified [these 48](https://github.com/search?q=repo%3Ahuggingface%2Ftransformers%20Processor(BaseImageProcessor)%3A&type=code) XXXImageProcessors that inherit from [BaseImageProcessor](https://github.com/huggingface/transformers/blob/main/src/transformers/image_processing_utils.py#L540), which may potentially use the modified `center_crop`. - CLIP is included there.

Running the tests for all of them `RUN_SLOW=1 pytest tests/models/xxxxx/image_processing_xxxxx.py`. Will put here the results. :crossed_fingers: 

@rafaelpadilla You only need to run the slow tests for models whose image processors use `center_crop` in their  `preprocess` method - [which should be ~20 models](https://github.com/search?q=repo%3Ahuggingface%2Ftransformers+path%3Amodels%2F*%2Fimage_processing_*.py+self.center_crop%28&type=code). 

> @rafaelpadilla You only need to run the slow tests for models whose image processors use `center_crop` in their `preprocess` method - [which should be ~20 models](https://github.com/search?q=repo%3Ahuggingface%2Ftransformers+path%3Amodels%2F*%2Fimage_processing_*.py+self.center_crop%28&type=code).

:heavy_check_mark: These are not affected by the changes:
```
RUN_SLOW=1 pytest tests/models/clip/*
RUN_SLOW=1 pytest tests/models/beit/*
RUN_SLOW=1 pytest tests/models/flava/*
RUN_SLOW=1 pytest tests/models/owlvit/*
RUN_SLOW=1 pytest tests/models/bridgetower/*
RUN_SLOW=1 pytest tests/models/videomae/*
RUN_SLOW=1 pytest tests/models/poolformer/*
RUN_SLOW=1 pytest tests/models/vit_hybrid/*
RUN_SLOW=1 pytest tests/models/deit/*
RUN_SLOW=1 pytest tests/models/chinese_clip/*
RUN_SLOW=1 pytest tests/models/efficientnet/*
RUN_SLOW=1 pytest tests/models/perceiver/*
```

:x: However, these models are affected by the fixes in the `center_crop()` and will have their tests failed:
```
RUN_SLOW=1 pytest tests/models/bit/* 
RUN_SLOW=1 pytest tests/models/levit/* 
RUN_SLOW=1 pytest tests/models/vivit/*
RUN_SLOW=1 pytest tests/models/mobilenet_v1/*
RUN_SLOW=1 pytest tests/models/mobilevit/*
RUN_SLOW=1 pytest tests/models/mobilenet_v2/*
RUN_SLOW=1 pytest tests/models/efficientformer/*
```

I'm glad we ran these tests! They've highlighted a significant trade-off: while the modifications in this PR address issue #22505, they also result in failing tests in 7 models. 
Maybe we should accept the different results in our `center_crop` and not merge this PR. :thinking: 

If I understand correctly, the results will be different only for image with odd number as size (or size diff)?
Do you know what kind of failures we have for `bit, mobilevit` etc. for this PR?
 

> If I understand correctly, the results will be different only for image with odd number as size (or size diff)? Do you know what kind of failures we have for `bit, mobilevit` etc. for this PR?

Yep. The results will be different if `image_height - crop_height` is odd or if `image_width - crop_width` is odd. This causes the crop of the image slightly different (~1 pixel shifted), resulting in these tests failures. 

Details for each case -> Note that all are related to this small shift.

**bit**
(top, bottm, left, right) was originally `(0, 448, 74, 522)` and now is `(0, 448, 75, 523)`
`tests.models.bit.test_modeling_bit::BitModelIntegrationTest.test_inference_image_classification_head()`

**levit**
(top, bottm, left, right) was originally `(16, 240, 58, 282)` and now is `(16, 240, 59, 283)`
`tests.models.levit.test_modeling_levit.py::LevitModelIntegrationTest::test_inference_image_classification_head()`

**vivit**
(top, bottm, left, right) was originally `(16, 240, 115, 339)` and now is `(16, 240, 116, 340)` (for the first frame, for instance)
`tests.models.vivit.test_modeling_vivit.py::VivitModelIntegrationTest::test_inference_for_video_classification()`

mobilenet_v1
(top, bottm, left, right) was originally (16, 240, 58, 282) and now is (16, 240, 59, 283)
tests.models.mobilenet_v1.test_modeling_mobilenet_v1.py::MobileNetV1ModelIntegrationTest::test_inference_image_classification_head

**mobilevit**
(top, bottm, left, right) was originally `(16, 528, 106, 618)` and now is `(16, 528, 107, 619)`
`tests.models.mobilevit.test_modeling_mobilevit.py::MobileViTModelIntegrationTest::test_inference_semantic_segmentation()`

**mobilenet_v2**
(top, bottm, left, right) was originally `(16, 529, 106, 619)` and now is `(16, 529, 107, 620)`
`tests.models.mobilenet_v2.test_modeling_mobilenet_v2.py::MobileNetV2ModelIntegrationTest::test_inference_semantic_segmentation()`

**efficientformer**
(top, bottm, left, right) was originally `(16, 240, 58, 282)` and now is `(16, 240, 59, 283)`
`tests.models.efficientformer.test_modeling_efficientformer.py::EfficientFormerModelIntegrationTest::test_inference_image_classification_head_with_teacher()`

Another possibility is to adapt the tests, so they can pass and fix #22505 .
However, we may have backward compatibility problems as this new version will produce different results than the previous one.

Thanks for the detailed information @rafaelpadilla . For me, we can change the tests to make them pass. For the backward compatibility, it is indeed tricky, especially CLIP is used everywhere now. Let's see what core maintainers @amyeroberts and @ArthurZucker say (especially Arthur has deal with some major changes in tokenizers recently).

@ydshieh @rafaelpadilla In general, I wouldn't be in favour of adding in this change because of how many users' models and outputs this would affect, in particular for CLIP which is one of our most popular models. The outputs compared with OpenAI are different, but they're not "wrong". 

Although at the moment we're slightly out-of-step with torchvision and its transformations, we don't have a 1:1 correspondence with the library. Most people using vision models in large training pipelines will also not be using the image processors. 

For reference, as noted in #22608, CLIP originally had the same cropping as torchvision, but this was (I believe accidentally) changed with #17628. 

@ArthurZucker WDYT? 

Yeah let's keep it BC but allow people to switch to the new behaviour as it does not affect everyone. Tokenizer is specific and I am no longer allowed to break it 😅  

> Yeah let's keep it BC but allow people to switch to the new behaviour as it does not affect everyone. Tokenizer is specific and I am no longer allowed to break it 😅

Could you share (or point to the place) how you did in tokenizers to `allow people to switch to the new behaviour` (I remember you have used something like `legacy` flag ..?)

Yes, [here](https://github.com/ArthurZucker/transformers/blob/72e9bd23250811083e8b2a37fd6143779d85cc51/src/transformers/models/llama/tokenization_llama.py#L154) it's set to None, and we raise a warning. I don't think we have to for CLIP, but just expose that a new behavior is available

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.