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

Remove color_space metadata and ConvertColorSpace() transform #7120

Merged
merged 17 commits into from
Jan 24, 2023

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Jan 20, 2023

As title.

The rationale for removing the color_space metadata is that there's a 1:1 mapping between the color-space and num_channels for all currently supported color-spaces. So color_space metadata is redundant. There's no plan to add support for additional color-spaces at this time (there could be in the future, at which time we could re-consider such meta-data).

The rationale for removing the ConvertColorSpace() transforms / functionals is that there's no obvious current need for those. We don't have such transforms in the current transforms. Obtaining an image with the desired color-space can currently be achieved by pil_to_tensor(Image.Open(...).convert("RGB")) for PIL images or read_image(..., mode=ImageReadMode.RGB) for tensors. (Also note the unfortunate overalp between the ColorSpace and ImageReadMode enums - something to consider in the future!)


Right now I'm being lazy and I moved the ColorSpace enum into the tests. The changes look big but it's mostly just s/datapoints.ColorSpace/ColorSpace/g.
However ideally, we probably want to get rid of ColorSpace completely and rely on num_channels instead. Waiting on @pmeier feedback on this first, as this seem to require pervasive changes throughout the tests which I'm not too familiar with yet.

Still TODO in another PR:

  • un-deprecate the [Random]GrayScale() transforms and make them rely on _rgb_to_gray().

cc @bjuncek @pmeier

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks Nicolas! Tests are failing because rgb_to_grayscale uses a function to determine the color space that was removed by you.

old_color_space = datapoints._image._from_tensor_shape(inpt.shape) # type: ignore[arg-type]

Since that is only used to construct the warning, we can just basically scrap it all together. I'll leave it up to you to actually un-deprecate here or in a follow-up PR.

@@ -238,7 +248,7 @@ def load(self, device):

@dataclasses.dataclass
class ImageLoader(TensorLoader):
color_space: datapoints.ColorSpace
color_space: ColorSpace
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can probably go, since it emulates the metadata on the actual datapoint.

Comment on lines 30 to 31
# TODO: Should this be `video` or can we remove it?
return cls._wrap(tensor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, that seems like an artifact. Basically the cls._to_tensor() and super().__new__ do the same thing. Let's remove the latter since it is obsolete.

Comment on lines 30 to +31

# TODO: in other PR (?) undeprecate those and make them use _rgb_to_gray?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's do that in a follow-up.

Comment on lines +1237 to +1239
# Looks like to_grayscale() is a stand-alone functional that is never called
# from the transform classes. Perhaps it's still here for BC? I can't be
# bothered to dig. Anyway, this can be deprecated as we migrate to V2.
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, the difference is that to_grayscale is just for PIL images and thus supports arbitrary input color spaces or rather all that Pillow supports. Although there is no functional difference for PIL images in rgb_to_grayscale, tensors have to have the three RGB channels.

+1 on deprecating.

@pmeier
Copy link
Collaborator

pmeier commented Jan 23, 2023

However ideally, we probably want to get rid of ColorSpace completely and rely on num_channels instead. Waiting on @pmeier feedback on this first, as this seem to require pervasive changes throughout the tests which I'm not too familiar with yet.

I agree, we shouldn't have enums in the test suite that don't exist in the library. Just to clarify: do you want to get rid of the enum or the general functionality? IMO, make_image(..., color_space="RGB") is quite a bit more expressive than make_image(..., num_channels=3).

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Lint job is failing: https://app.circleci.com/jobs/github/pytorch/vision/1810524. Otherwise LGTM, thanks Nicolas!

@NicolasHug NicolasHug merged commit 5dd9594 into pytorch:main Jan 24, 2023
@github-actions
Copy link

Hey @NicolasHug!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

@vadimkantorov
Copy link

vadimkantorov commented Jan 26, 2023

The rationale for removing the ConvertColorSpace() transforms / functionals is that there's no obvious current need for those.

colorspace conversion functions are useful in contexts other than augmentations, related discussion: #4029 and such functions are present in most computer vision libraries

facebook-github-bot pushed a commit that referenced this pull request Feb 8, 2023
…sform (#7120)

Reviewed By: vmoens

Differential Revision: D43116108

fbshipit-source-id: ea112dba0cff07499a4ead53bc19526fd81c962f
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.

None yet

4 participants