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

make datapoint methods private #7733

Closed
wants to merge 1 commit into from
Closed

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jul 11, 2023

Reasons:

  1. They should not have been public in the first place. Although they are undocumented, users can still do Image(...).crop(...) and might think this is normal way to do it.
  2. They clash with the PyTorch core ops. As of now, this only happens for Datapoint.resize, which overwrites Tensor.resize. However, there might be more of these in the future.

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 11, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7733

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 58b8eba:

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

def _gaussian_blur(self, kernel_size: List[int], sigma: Optional[List[float]] = None) -> Datapoint:
return self

def _normalize(self, mean: List[float], std: List[float], inplace: bool = False) -> Datapoint:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was missed in #7113. We technically don't need it, since both the dispatcher

elif isinstance(inpt, (datapoints.Image, datapoints.Video)):
return inpt.normalize(mean=mean, std=std, inplace=inplace)
else:
raise TypeError(
f"Input can either be a plain tensor or an `Image` or `Video` datapoint, " f"but got {type(inpt)} instead."
)

and the transform

_transformed_types = (datapoints.Image, is_simple_tensor, datapoints.Video)

filter on the datapoint type. However, this is the only dispatcher that does not have a method defined on the datapoint base class.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks Philip, LGTM if green

I guess now we need an "official" way of supporting user-defined Datapoints. But we can start thinking about that later.

@pmeier pmeier mentioned this pull request Jul 11, 2023
@pmeier
Copy link
Collaborator Author

pmeier commented Jul 28, 2023

Superseded by #7747.

@pmeier pmeier closed this Jul 28, 2023
@pmeier pmeier deleted the datapoints-private branch July 28, 2023 09:47
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.

4 participants