-
Notifications
You must be signed in to change notification settings - Fork 27.7k
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
Add Mask R-CNN #25348
Add Mask R-CNN #25348
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
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.
Thanks for iterating on this!
I've done an initial review - commenting on the things that caught my eye. It wasn't however, extensive and I expect this PR will still need several rounds of review.
General comments:
- Could you add type hints to arguments in the functions?
- Make sure all doc strings are filled out fully, including: descriptions of what the argument is, any shape information, shape information with explicit variable names
- Given the complexity of this model, it needs a lot more tests beyond the defaults in the testing suite. In particular for implementations of loss averaging and NMS
N
is used everywhere - it should be replaced with something more explicit- As this PR is so necessarily big, I would try to find ways to make it as small as possible and then have follow up PRs for additional functionality. In particular, it would be a lot easier to review without the conditional onnx logic. Could you remove this and then add in a separate PR please?
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.
Thanks for iterating on this!
I've done an initial review - commenting on the things that caught my eye. It wasn't however, extensive and I expect this PR will still need several rounds of review.
General comments:
- Could you add type hints to arguments in the functions?
- Make sure all doc strings are filled out fully, including: descriptions of what the argument is, any shape information, shape information with explicit variable names
- Given the complexity of this model, it needs a lot more tests beyond the defaults in the testing suite. In particular for implementations of loss averaging and NMS
N
is used everywhere - it should be replaced with something more explicit- As this PR is so necessarily big, I would try to find ways to make it as small as possible and then have follow up PRs for additional functionality. In particular, it would be a lot easier to review without the conditional onnx logic. Could you remove this and then add in a separate PR please?
13a150a
to
346b096
Compare
13ab617
to
a20d9cf
Compare
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
a20d9cf
to
901d7f9
Compare
What does this PR do?
This PR is a further development of the Mask R-CNN framework. Supersedes #22973.
Updates:
__repr__
and__nice__
methodstorchvision
was already leveraged for NMSRegarding this:
=> this is because when placing masks on the GPU, this would cause OOM errors. Hence those are placed on the CPU.