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

Use Sequence for parameters type checking in transforms.RandomErase #8549

Closed
lqrhy3 opened this issue Jul 29, 2024 · 3 comments
Closed

Use Sequence for parameters type checking in transforms.RandomErase #8549

lqrhy3 opened this issue Jul 29, 2024 · 3 comments

Comments

@lqrhy3
Copy link

lqrhy3 commented Jul 29, 2024

🚀 The feature

Use typing.Sequence to check types of sample and ratio arguments instead of (list, tuple) in transforms.RandomErase class.

Motivation, pitch

As I see all other transforms use typing.Sequence to ensure that sequences were passed where needed (e.g. Normalize, transforms.RandomResizedCrop and so on). However isinstance(scale, (tuple, list)) (and same for ratio) is used in RandomErase.
This, for example, leads to impossibility of instantiating this transform from config using hydra.instantiate, because scale then has OmegaConf.ListConfig type which is a Sequence, but not tuple or float and causes TypeError.

Alternatives

No response

Additional context

No response

@NicolasHug
Copy link
Member

Thanks for the proposal @lqrhy3, happy to consider a PR. Sometimes, we have to use weird / wrong type check and annotation because of torchscript support. Not sure if that applies here, but that could be a reason why it was done this way in the first place.
Also note that we should mostly just update transforms.v2.RandomErasing instead of transforms.RandomErasing. We can update the latter if it doesn't take too much effort, but the old "v1" namespace is discouraged in favor of v2 now.

@venkatram-dev
Copy link
Contributor

created PR #8615

@NicolasHug
Copy link
Member

Looks like this can be closed now that #8615 is merged. Thanks @lqrhy3 for the issue and @venkatram-dev for the PR!

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

No branches or pull requests

3 participants