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

Fix: siglip image processor rgb_convert is not being applied correctly. #34301

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

jp1924
Copy link
Contributor

@jp1924 jp1924 commented Oct 22, 2024

What does this PR do?

        # All transformations expect numpy arrays.
        images = [to_numpy_array(image) for image in images]

        if do_convert_rgb:
            images = [convert_to_rgb(image) for image in images]

The convert_to_rgb function is not being applied because ndarray values are being input
(convert_to_rgb simply returns the input value if it's not a PIL.Image).

If an image with RGBA or similar format is input, the conversion doesn't work properly,
causing the following error in the infer_channel_dimension_format method:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/transformers/image_utils.py", line 254, in infer_channel_dimension_format
    raise ValueError("Unable to infer channel dimension format")
ValueError: Unable to infer channel dimension format

transformers version: 4.46.0.dev0

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?

Who can review?

@amyeroberts

@LysandreJik LysandreJik requested review from qubvel and molbap October 22, 2024 14:24
Copy link
Contributor

@molbap molbap left a comment

Choose a reason for hiding this comment

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

Good catch @jp1924 ! - I checked, it seems Siglip was the only exception with the wrong order of operations.

@jp1924
Copy link
Contributor Author

jp1924 commented Oct 24, 2024

@molbap
Oh, thank you for reviewing.
Separately, I noticed that do_convert_rgb is not implemented in ViTImageProcessor. Was this intentional?

@molbap
Copy link
Contributor

molbap commented Oct 24, 2024

There's no particular reason why - I have no opposition to introducing this in another PR :)

@jp1924
Copy link
Contributor Author

jp1924 commented Oct 27, 2024

@molbap
Oh yeah? So I can just open the PR and add it?

@molbap
Copy link
Contributor

molbap commented Oct 28, 2024

@jp1924, sure, no opposition to it - the main thing we are careful about is not breaking backwards compatibility. In this case, no previous behaviour can be broken since you're adding a functionality, so feel free to open a PR :)

Copy link
Member

@qubvel qubvel 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 the fix, indeed convert_rgb is not supported for numpy arrays! I suppose we can merge this fix!

Also would be great to update convert_rgb to avoid silently skipping numpy arrays.

@qubvel qubvel requested a review from LysandreJik October 30, 2024 10:36
@jp1924 jp1924 mentioned this pull request Oct 31, 2024
5 tasks
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

thanks 🤗

@ArthurZucker ArthurZucker merged commit 3cd78be into huggingface:main Nov 19, 2024
8 checks passed
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
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