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

[NOMRG] TransformsV2 TODOs #7082

Closed
wants to merge 2 commits into from

Conversation

NicolasHug
Copy link
Member

Just keeping track of a bunch of TODOs following up on conversations with @pmeier and @vfdev-5 .

torchvision/prototype/transforms/_transform.py Outdated Show resolved Hide resolved
Comment on lines +247 to +248
# TODO: Figure out whether this cond could just be:
# if torch.jit.is_scripting() or is_simple_tensor(inpt):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works out 🚀 Gonna send a PR soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #7084

@@ -1,5 +1,6 @@
from ._bounding_box import BoundingBox, BoundingBoxFormat
from ._datapoint import FillType, FillTypeJIT, InputType, InputTypeJIT
from ._datapoint import FillType, FillTypeJIT, InputType, InputTypeJIT # TODO: these may not need to be public?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most likely not. We only introduced them for internal convenience. Whenever you see something annotates with *JIT, you can basically look up the eager counterpart to see what is actually supported.

torchvision/prototype/datapoints/_bounding_box.py Outdated Show resolved Hide resolved
Comment on lines +61 to +62
# For now, this is somewhat redundant with number of channels.
# TODO: decide whether we want to keep it?
Copy link
Collaborator

Choose a reason for hiding this comment

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

For context, this was designed with the use case of other color spaces with the same number of channels. For example, #4029. Since we can always add this later on, I see no harm of removing it now. That being said, I would keep the ColorSpace enum for

which is a generalized replacement for

def to_grayscale(inpt: PIL.Image.Image, num_output_channels: int = 1) -> PIL.Image.Image:

and

Copy link
Collaborator

Choose a reason for hiding this comment

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

While going through our source again, I noticed this:

if isinstance(inpt, torch.Tensor) and (
torch.jit.is_scripting() or not isinstance(inpt, (datapoints.Image, datapoints.Video))
):
if old_color_space is None:
raise RuntimeError(
"In order to convert the color space of simple tensors, "
"the `old_color_space=...` parameter needs to be passed."
)
return convert_color_space_image_tensor(inpt, old_color_space=old_color_space, new_color_space=color_space)

As long as we can accurately identify the color space from the number of channels, we should either remove the old_color_space parameter completely or at least remove this error and set the value ourselves with

def _from_tensor_shape(shape: List[int]) -> ColorSpace:

format: BoundingBoxFormat
spatial_size: Tuple[int, int]
format: BoundingBoxFormat # TODO: do not use a builtin?
spatial_size: Tuple[int, int] # TODO: This is the size of the image, not the box. Should we make the name more obvious?
Copy link
Collaborator

Choose a reason for hiding this comment

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

For context #6736

@@ -35,6 +35,9 @@ def forward(self, *inputs: Any) -> Any:

params = self._get_params(flat_inputs)

# TODO: right now, any tensor or datapoint passed to forward() will be transformed.
# The rest is bypassed.
# What if there are tensor parameters that users don't want to be transformed? Is that a plausible scenario?
Copy link
Collaborator

Choose a reason for hiding this comment

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

We previously agreed that we should have a no-op datapoint that will never be transformed. There was some discussion to use the plain Datapoint class directly or add another thin wrapper subclass as suggested in #6663 (comment)

@NicolasHug
Copy link
Member Author

Closing - we'll keep track of the progress in other places like #7217 (possibly coming back to this PR)

@NicolasHug NicolasHug closed this Feb 10, 2023
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.

3 participants