Skip to content

fix RandomGrayscale for grayscale inputs #5585

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

Closed
wants to merge 1 commit into from

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Mar 10, 2022

Fixes #5581. With this fix we don't need to touch F.rgb_to_grayscale. The mismatch between tensor and PIL kernel is still open, but somewhat irrelevant for this. The documentation as well as the name of F.rgb_to_grayscale clearly state that this is converting RGB inputs. Thus, having mismatching but not wrong behavior for grayscale inputs is not a major problem, since the user is already misusing the kernel. Plus, this whole issue will go away for the prototype transforms, since we will handle color space conversions in a general way there.

RandomGrayscale relied on the undocumented fact that F.rgb_to_grayscale is a no-op for grayscale PIL. As shown in #5581, this assumption does not hold for tensor images. This patch removes this assumption and handles grayscale inputs explicitly.

@facebook-github-bot
Copy link

facebook-github-bot commented Mar 10, 2022

💊 CI failures summary and remediations

As of commit 93d2d5a (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@datumbox
Copy link
Contributor

Thanks Philip. I confirm that this is a viable candidate solution.

As discussed offline, this approach solves the situation for the Class Transform but maintains the difference in behaviour between PIL and Tensor backends for F.rgb_to_grayscale.

I think there are other candidate solutions with other pros/cons worth discussing. As this is not a priority right now and due to lack of time, let's postpone the discussion for later. To avoid having an open unreviewed PR, I'm going to close it. If after our discussion this becomes the selected approach, we will simply reopen the PR and merge.

@datumbox datumbox closed this Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RandomGrayscale fails for grayscale tensor inputs
3 participants