-
Notifications
You must be signed in to change notification settings - Fork 465
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
feat: Added loading method for PyTorch artefact detection models from HF Hub #836
Conversation
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 this amazing feature! just a small comment because you modified a parameter in the refacto
@@ -31,11 +29,11 @@ def _fasterrcnn(arch: str, pretrained: bool, **kwargs: Any) -> FasterRCNN: | |||
"image_mean": default_cfgs[arch]['mean'], | |||
"image_std": default_cfgs[arch]['std'], | |||
"box_detections_per_img": 150, | |||
"box_score_thresh": 0.15, | |||
"box_score_thresh": 0.5, |
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.
Are you positive this change doesn't decrease performances ?
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 experimented a bit with the model as is, and this certainly decreases the recall, but the bare predictions got me concerned on the precision side. @SiddhantBahuguna suggested having a class-specific threshold strategy and that might be the best thing to do, but for now, perhaps it might be better to maximize our precision.
Happy to revert this if you think it's best to favor recall @charlesmindee 👌
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.
Let's put it to 0.5 for now, but we should implement quickly a threshold by class if it leads to better results
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, @SiddhantBahuguna could you check this threshold by class when you have the time to do so ?
@fg-mindee Can you also add a section into the docs for this feature ? 🤗 Maybe where we can add later links to some models on the hub (this would also fix the different vocab pretrained support i think) |
Codecov Report
@@ Coverage Diff @@
## main #836 +/- ##
==========================================
+ Coverage 95.91% 95.94% +0.03%
==========================================
Files 131 133 +2
Lines 5086 5103 +17
==========================================
+ Hits 4878 4896 +18
+ Misses 208 207 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Hey Felix 👋 This is something we have to address but the documentation is built using the TF backend for now (since most high-level feature are identical) and this PR is only about PyTorch (we have no implementation of faster rcnn in TF in docTR for now) 😅 |
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!
Hi @fg-mindee @charlesmindee, actually, since we are using the bare output of the model it lacks post nms strategy. |
Following up on #426, this PR introduces the following modifications:
from_hub
function to load artefact detection architectures in PyTorchWith PyTorch backend, the following snippets are completely equivalent:
Any feedback is welcome!