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

feat: Added loading method for PyTorch artefact detection models from HF Hub #836

Merged
merged 11 commits into from
Mar 8, 2022

Conversation

fg-mindee
Copy link
Contributor

@fg-mindee fg-mindee commented Feb 25, 2022

Following up on #426, this PR introduces the following modifications:

  • refactored faster rcnn
  • added a from_hub function to load artefact detection architectures in PyTorch
  • added unittest
  • fixed header unittest

With PyTorch backend, the following snippets are completely equivalent:

# Pretrained
from doctr.models.obj_detection import fasterrcnn_mobilenet_v3_large_fpn
model = fasterrcnn_mobilenet_v3_large_fpn(pretrained=True)
# HF Hub
from doctr.models.obj_detection.factory import from_hub
model = from_hub("mindee/fasterrcnn_mobilenet_v3_large_fpn")

Any feedback is welcome!

@fg-mindee fg-mindee added module: models Related to doctr.models ext: tests Related to tests folder framework: pytorch Related to PyTorch backend topic: object detection Related to the task of object detection type: new feature New feature labels Feb 25, 2022
@fg-mindee fg-mindee added this to the 0.6.0 milestone Feb 25, 2022
@fg-mindee fg-mindee self-assigned this Feb 25, 2022
Copy link
Collaborator

@charlesmindee charlesmindee 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 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,
Copy link
Collaborator

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 ?

Copy link
Contributor Author

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 👌

Copy link
Collaborator

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

charlesmindee
charlesmindee previously approved these changes Mar 7, 2022
Copy link
Collaborator

@charlesmindee charlesmindee left a 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 ?

@felixdittrich92
Copy link
Contributor

@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
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #836 (0c7fc40) into main (c9806fa) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests 95.94% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
doctr/models/obj_detection/faster_rcnn/pytorch.py 100.00% <ø> (ø)
doctr/models/obj_detection/factory/__init__.py 100.00% <100.00%> (ø)
doctr/models/obj_detection/factory/pytorch.py 100.00% <100.00%> (ø)
doctr/transforms/modules/base.py 94.59% <0.00%> (ø)
doctr/transforms/functional/base.py 97.10% <0.00%> (+1.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9806fa...0c7fc40. Read the comment docs.

@fg-mindee
Copy link
Contributor Author

@fg-mindee Can you also add a section into the docs for this feature ? hugs Maybe where we can add later links to some models on the hub (this would also fix the different vocab pretrained support i think)

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) 😅

Copy link
Collaborator

@charlesmindee charlesmindee left a comment

Choose a reason for hiding this comment

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

Thanks!

@fg-mindee fg-mindee merged commit 9b31588 into main Mar 8, 2022
@fg-mindee fg-mindee deleted the obj-det branch March 8, 2022 09:49
@SiddhantBahuguna
Copy link
Contributor

SiddhantBahuguna commented Mar 8, 2022

Thanks, @SiddhantBahuguna could you check this threshold by class when you have the time to do so ?

Hi @fg-mindee @charlesmindee, actually, since we are using the bare output of the model it lacks post nms strategy.
After observing the output, along with other filter methods I did use thresholds for logos specifically and it did lead to better precision and recall. I will open a PR soon for the same :)

@frgfm frgfm mentioned this pull request Jun 28, 2022
85 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: tests Related to tests folder framework: pytorch Related to PyTorch backend module: models Related to doctr.models topic: object detection Related to the task of object detection type: new feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants