Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added typing annotations to models/detection [1/n] #4220
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
Added typing annotations to models/detection [1/n] #4220
Changes from all commits
b2f6615
4fb038d
deda5d7
5490821
4cfc220
6306746
e8c93cf
6871ccc
53fe949
ecc58a7
6a30c92
fb3ea88
1f5f715
d13311d
5adda72
d16be19
4580541
1ba33ce
254e51b
47f75dc
6aff88c
516fb68
b026039
18ec557
24e3f74
ab7c671
ff5698a
50641d5
6a9c0cb
700d74d
c0e836b
0a93c17
a0b3b2a
fc8032c
730a33d
4cd5b84
c278189
fe7e289
1fda3e4
0daf0d8
4fa9294
f9c7fbe
f1eeea1
12e7d7b
eae251f
9d0d6cf
e437b60
a6bb70c
76b26ba
623d5e4
bb73a22
4e8e9f7
ca5d600
8976768
6fc8812
7422d3b
a10588c
5f2dcc4
70e5b6a
7d4e919
01fdb48
cace5ff
e941e48
623017b
becbbd6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
box_head: Optional[RPNHead] = None
I believeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was bit unsure on these. I think that user can write their own classes and just pass them to
FasterRCNN
.Hence I thought it shouldn't be so particular class like
RPNHead
.Let me know, I will change all of these otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we allow passing custom classes, we need structural duck typing here. Meaning we need to define a custom
typing.Protocol
that defines how these objects need to look like. @oke-aditya can you make a list of all the attributes we access and methods we call on these objects?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure of what you mean @pmeier . I'm new to type hints.
I think trying to be very strict in typing by custom classes might be too much. If
nn.Module
satisfies the purpose I think it should be good enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. As a rule of thumb, the input types should be as loose as possible and the output types as strict as possible.
After some digging, I think
vision/torchvision/models/detection/roi_heads.py
Line 753 in fe3118c
is the only time we do anything with the object passed as
box_head
. Since it is possible to call annn.Module
, using it as annotation is fine.In other cases @frgfm's comment might have some merit in other cases: for example, if we at some point access
box_head.foo
,nn.Module
is not sufficient as annotation, since it doesn't define afoo
attribute. In such a case we have two options:RPNHead
that has this attribute.typing.Protocol
that defines the interface we are looking for. In the example above the object passed tobox_head
needs to be callable and define an attributefoo
.Especially if users are allowed to pass custom objects, I would always prefer 2. over 1. because it doesn't require any changes on the user side. I don't know if this works with torchscript though.
Let's discuss this if the need for something like this arises.