-
Notifications
You must be signed in to change notification settings - Fork 211
add predict_kwargs in ObjectDetectionModel in order to filter the pre… #990
add predict_kwargs in ObjectDetectionModel in order to filter the pre… #990
Conversation
…diction using custom treshold and manage other parameters
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## master #990 +/- ##
==========================================
- Coverage 88.45% 88.40% -0.06%
==========================================
Files 250 250
Lines 13158 13190 +32
==========================================
+ Hits 11639 11660 +21
- Misses 1519 1530 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Awesome, looking great! @Actis92 mind also adding this to the instance segmentation and keypoint detection tasks? They are also using the IceVision adapter under the hood 😃
…_params' into feature/954_icevision_additional_params
for more information, see https://pre-commit.ci
@ethanwharris I have added this parameter to the instance segmentation and keypoint detection tasks. Let me know if it's needed something more :) |
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.
Awesome, I think we could make the setter slightly cleaner but just a minor point 😃 Also, mind updating the CHANGELOG?
flash/image/detection/model.py
Outdated
def set_predict_kwargs(self, value): | ||
"""This function is used to update the kwargs used for the prediction step.""" | ||
self.adapter.predict_kwargs = value |
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 think it would be cleaner to do this with properties. Something like:
def set_predict_kwargs(self, value): | |
"""This function is used to update the kwargs used for the prediction step.""" | |
self.adapter.predict_kwargs = value | |
@property | |
def predict_kwargs(self) -> Dict[str, Any]: | |
"""The kwargs used for the prediction step.""" | |
return self.adapter.predict_kwargs | |
@predict_kwargs.setter | |
def predict_kwargs(predict_kwargs: Dict[str, Any]): | |
self.adapter.predict_kwargs = predict_kwargs |
One day we should find a way for this sort of thing to exist on the adapter and automatically be attached here...
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.
Hi @ethanwharris I will do the changes, both CHANGELOG and the properties. For what concern how to do it automatically, I think that a way is that to automatically update the attribute __dict__
of the class, taking the properties from the adapter or something like that :)
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 have done the changes
… and update CHANGELOG.md
for more information, see https://pre-commit.ci
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.
Nice! LGTM 😃
…diction using custom threshold and manage other parameters
What does this PR do?
In this PR I have added the possibility to specify predict_kwargs for ObjectDetector that can be used to add additional parameters that are used during the prediction step. For example is possible to specify a custom threshold in order to filter the bounding boxes predicted by Icevision. And added setter method in order to allow user to update the values
Fixes #954