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

Expose misc ops at package level #4812

Merged
merged 19 commits into from
Nov 2, 2021
Merged

Conversation

jdsgomes
Copy link
Contributor

@jdsgomes jdsgomes commented Nov 1, 2021

@facebook-github-bot
Copy link

facebook-github-bot commented Nov 1, 2021

💊 CI failures summary and remediations

As of commit 89c1c0b (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

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!

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 for the PR @jdsgomes ,

If we're going to expose these in the torchvision.ops namespace, I think we should also expose them in the docs in ops.rst. These ops seem to lack docstring though, so maybe we should write some

@oke-aditya
Copy link
Contributor

oke-aditya commented Nov 1, 2021

We were going to expose using torchvision.nn ? Or is it decided with ops.misc.py ?

@datumbox
Copy link
Contributor

datumbox commented Nov 1, 2021

@oke-aditya The reason we are not doing it at the moment is because there is no consensus for renaming/moving stuff to nn. This PR is just addressing the fact that the classes are public but not exposed directly via ops. Though the RFC you send is still valid, I would recommend not to link the two so that we don't block fixing outstanding issues.

@oke-aditya
Copy link
Contributor

Can understand it :) thanks for the clarification.

@jdsgomes
Copy link
Contributor Author

jdsgomes commented Nov 1, 2021

Thanks for the PR @jdsgomes ,

If we're going to expose these in the torchvision.ops namespace, I think we should also expose them in the docs in ops.rst. These ops seem to lack docstring though, so maybe we should write some

Do you suggest adding the docstrings in this PR as well or that is out of scope and we should create a ticket for it?

@NicolasHug
Copy link
Member

It's a small PR so I would suggest to do it here. It's fine to do it in a separate PR, as long it's not a "open an issue for it and forget" thing :)

@datumbox
Copy link
Contributor

datumbox commented Nov 1, 2021

@jdsgomes if you feel you don't have context to write the docs, feel free to open a ticket and I'll help.

Here are some info in case you want to give it a try:

The ConvNormActivation is a common configurable block used for Convolution-Normalzation-Activation blocks. We should document the default values along with what happens if None are passed for bn and activation (the layers are skipped).

The SqueezeExcitation is another common block first introduced at https://arxiv.org/abs/1709.01507. For this we just need to talk about the default values.

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.

Overall looks good. Just one comment on the stride size type. Let me know what you think.

torchvision/ops/misc.py Outdated Show resolved Hide resolved
@jdsgomes jdsgomes merged commit d24ef4c into pytorch:main Nov 2, 2021
facebook-github-bot pushed a commit that referenced this pull request Nov 6, 2021
Summary:
* Expose misc ops at package level

* Adding documentation to the ops exposed

Reviewed By: kazhang

Differential Revision: D32216670

fbshipit-source-id: 63d39b4e9b07020b29a399d0a1ae7bbb5f26bdbf
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
* Expose misc ops at package level

* Adding documentation to the ops exposed
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.

5 participants