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

Detection model #32

Merged
merged 35 commits into from
Jan 22, 2021
Merged

Detection model #32

merged 35 commits into from
Jan 22, 2021

Conversation

charlesmindee
Copy link
Collaborator

DetectionModel class and DBModel (child class) added.
DB is implemented with a ResNet50 backbone (feature extactor) provided by keras

@charlesmindee charlesmindee added the module: models Related to doctr.models label Jan 21, 2021
@fg-mindee fg-mindee added this to the 0.1.0 milestone Jan 21, 2021
Copy link
Contributor

@fg-mindee fg-mindee 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! I added a few comments

@@ -1,5 +1,4 @@
# Copyright (C) 2021, Mindee.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to keep this line break


class DetectionModel(keras.Model):
"""Implements abstract DetectionModel class

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing args here

self,
inputs: tf.Tensor,
training: bool = False
) -> Tuple[keras.Model, keras.Model]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Legacy typing here

@@ -161,3 +164,248 @@ def __call__(

bounding_boxes.append(boxes_batch)
return bounding_boxes


class DBModel(DetectionModel, keras.Model):
Copy link
Contributor

Choose a reason for hiding this comment

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

The inherited DetectionModel already inherits from keras.Model

Also, since we will use ResNet50 for now, let's name it DBResNet50

Comment on lines 94 to 96
assert np.shape(dboutput_notrain.numpy())[0] == 8
# output dimensions
assert np.shape(dboutput_notrain.numpy())[1] == np.shape(dboutput_notrain.numpy())[2] == 640
Copy link
Contributor

Choose a reason for hiding this comment

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

you can regroup shape value testing:

assert dboutput_notrain.numpy().shape == (8, 640, 640, 3)

Comment on lines 102 to 104
assert all(np.shape(maps.numpy())[0] == 8 for maps in dboutput_train)
# output dimensions
assert all(np.shape(maps.numpy())[1] == np.shape(maps.numpy())[2] == 640 for maps in dboutput_train)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, this can be grouped (and np.shape is not required, you can use the .shape method of the ndarray)

channels: int = 128,
) -> None:
super().__init__(shape)
self.channels = channels
Copy link
Contributor

Choose a reason for hiding this comment

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

Many modules could be instantiated over here

@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #32 (c6e13d9) into main (1905d27) will not change coverage.
The diff coverage is 98.46%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #32   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files          12       13    +1     
  Lines         256      320   +64     
=======================================
+ Hits          252      315   +63     
- Misses          4        5    +1     
Impacted Files Coverage Δ
doctr/models/detection/postprocessor.py 90.90% <ø> (ø)
doctr/models/detection/model.py 90.00% <90.00%> (ø)
doctr/models/detection/__init__.py 100.00% <100.00%> (ø)
...tr/models/detection/differentiable_binarization.py 99.17% <100.00%> (+0.64%) ⬆️

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 1905d27...0615752. Read the comment docs.

Copy link
Contributor

@fg-mindee fg-mindee left a comment

Choose a reason for hiding this comment

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

Sorry about the number of comments 😅

doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
Copy link
Contributor

@fg-mindee fg-mindee left a comment

Choose a reason for hiding this comment

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

Just a few adjustments left 😅
It seems we burned out our free CI minutes

doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
doctr/models/detection/differentiable_binarization.py Outdated Show resolved Hide resolved
Copy link
Contributor

@fg-mindee fg-mindee 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 edits! Looks good to me!

@fg-mindee fg-mindee merged commit b04a219 into main Jan 22, 2021
@fg-mindee fg-mindee deleted the detection_model branch January 22, 2021 11:15
@fg-mindee fg-mindee mentioned this pull request Jan 22, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: models Related to doctr.models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants