Skip to content

[proto] Clean-up Label.to_categories #6419

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

Merged
merged 3 commits into from
Aug 15, 2022
Merged

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Aug 15, 2022

No description provided.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!


return apply_recursively(lambda idx: cast(Sequence[str], self.categories)[idx], self.tolist())
return tree_map(lambda idx: self.categories[idx], self.tolist())
Copy link
Member

Choose a reason for hiding this comment

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

We have a lot of calls to apply_recursively, should we update them all?

Copy link
Member

@NicolasHug NicolasHug Aug 15, 2022

Choose a reason for hiding this comment

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

but also, _pytree looks private - should we actually use it ?

Copy link
Collaborator Author

@vfdev-5 vfdev-5 Aug 15, 2022

Choose a reason for hiding this comment

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

Yes, in transforms I already replaced almost all of them (remains only BatchMultiCrop)

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.

LGTM if CI is green.

@github-actions
Copy link

Hey @vfdev-5!

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

@NicolasHug
Copy link
Member

@vfdev-5 @pmeier @datumbox any thought on #6419 (comment) ?

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Aug 15, 2022

but also, _pytree looks private - should we actually use it ?

It is used in functorch eager ops (e.g. vmap: https://github.com/pytorch/functorch/blob/518fecce963846679add02ad36a65d36d59a362d/functorch/_src/vmap.py#L11), so I would expect to be able to use it
here as well. cc @zou3519

@NicolasHug
Copy link
Member

I believe functorch is bound to be integrated within core eventually, so they don't operate under the same constraints as we do.

We probably want to be careful about using private stuff from core because it will not just break us on GitHub; most importantly, if a Meta engineer modifies that private API internally, they'll start seeing the torchvision tests fail for no good reason. Nobody wants to see their code fail just because someone else has been using a private API from their repo.

@zou3519
Copy link
Contributor

zou3519 commented Aug 15, 2022

functorch isn't the only library that uses _pytree, PyTorch Core features (like torch.fx) use _pytree utilities as well.

We haven't exposed _pytree publicly yet, but if you want to use it and exposing it would make you feel more comfortable with using that, then please let us know. The reason why it is prefixed with an underscore is because we have not done work to expose it as a public API.

@NicolasHug
Copy link
Member

We haven't exposed _pytree publicly yet, but if you want to use it and exposing it would make you feel more comfortable with using that, then please let us know

Great, thanks @zou3519

@vfdev-5 would you mind submitting a feature request to core for tree_map() to be public?

@zou3519
Copy link
Contributor

zou3519 commented Aug 15, 2022

There's an issue already over at pytorch/pytorch#65761

@NicolasHug
Copy link
Member

@vfdev-5 Out of curiosity what was the original reason to go from apply_recursively to tree_map()? Looking at pytorch/pytorch#65761 tree_map() is said to be slow (not sure if that's still the case, or whether that's something critical for us)

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Aug 15, 2022

Performance issue may be resolved from core side. From torchvision point of view keeping our own implementations (e.g. apply_recursive and related methods) would increase codebase maintenance.

@NicolasHug we also using tree_flatten and tree_unflatten: https://github.com/pytorch/vision/blob/main/torchvision/prototype/transforms/_transform.py

facebook-github-bot pushed a commit that referenced this pull request Aug 24, 2022
Summary:
* [proto] Clean-up Label.to_categories

* Fixed flake8

Reviewed By: datumbox

Differential Revision: D38824230

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

6 participants