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
Fix random bounding box crop #512
Conversation
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
@@ -129,20 +133,25 @@ void RandomBBoxCrop<CPUBackend>::RunImpl(SampleWorkspace *ws, const int) { | |||
labels.emplace_back(*label_data); | |||
} | |||
|
|||
const auto prospective_crop = | |||
FindProspectiveCrop(bounding_boxes, labels, SelectMinimumOverlap()); | |||
while (true) { |
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.
Use some reasonable value for max. number of attempts (100?) if allow_no_crop
is not specified - otherwise this function may hang.
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'm a bit worried about changing this default value, as it severely degrades training accuracy. Additionally, with default threshold 0
and default allow_no_crop
to true
the problem should be somewhat alleviated. Maybe I add some info about this to op doc string? To be careful with these parameters and why.
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.
while(true)? where is the break condition?
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.
There was a return
statement, but the whole thing was rather clumsy. I added constructor for ProspectiveCrop
and refactored it to make it more readable and easier to follow.
Signed-off-by: Albert Wolant <awolant@nvidia.com>
@@ -129,20 +133,25 @@ void RandomBBoxCrop<CPUBackend>::RunImpl(SampleWorkspace *ws, const int) { | |||
labels.emplace_back(*label_data); | |||
} | |||
|
|||
const auto prospective_crop = | |||
FindProspectiveCrop(bounding_boxes, labels, SelectMinimumOverlap()); | |||
while (true) { |
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.
while(true)? where is the break condition?
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
3aa87fe
to
b21c4fd
Compare
Builds 624482 |
Signed-off-by: Albert Wolant <awolant@nvidia.com>
3454354
to
ee9932b
Compare
Builds 625254 |
876b28a
to
000b59d
Compare
Signed-off-by: Albert Wolant <awolant@nvidia.com>
000b59d
to
b9b8c89
Compare
Builds 625321 |
* Fix RandomBBox crop Signed-off-by: Albert Wolant <awolant@nvidia.com> * Add sample options Signed-off-by: Albert Wolant <awolant@nvidia.com> * Fix lint errors Signed-off-by: Albert Wolant <awolant@nvidia.com> * Add ProspectiveCrop as struct Signed-off-by: Albert Wolant <awolant@nvidia.com> * Refactored FindProspectiveCrop calls Signed-off-by: Albert Wolant <awolant@nvidia.com> * Fixed indentations Signed-off-by: Albert Wolant <awolant@nvidia.com> * Add comment to explain sample_options_ Signed-off-by: Albert Wolant <awolant@nvidia.com> * Improved docs Signed-off-by: Albert Wolant <awolant@nvidia.com> * Removed _ from fields names in ProspectiveCrop Signed-off-by: Albert Wolant <awolant@nvidia.com> * Refactor ProspectiveCrop defautl ctor Signed-off-by: Albert Wolant <awolant@nvidia.com> * Address review comments Signed-off-by: Albert Wolant <awolant@nvidia.com>
No description provided.