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.

Sign up for GitHub

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

Jump to bottom

Add Fast NMS. #2410

Merged

KimSangYeon-DGU merged 6 commits into mlpack:master from kartikdutt18:Add-NMS

May 23, 2020

Member

kartikdutt18 commented May 17, 2020 •

edited

Hey everyone,
Currently this WIP implementation of Fast Non Maximal Suppression.

Things left to do:

Add tests.
Add more comments.
Add size checks.

Reference implementation
Test - Link

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels

favre49 added c: methods t: added feature and removed s: unlabeled labels

kartikdutt18 changed the title ~~[WIP] Add Fast NMS.~~ Add Fast NMS.

kartikdutt18 force-pushed the Add-NMS branch from a6fd478 to 78fdfdb Compare

May 19, 2020 07:10

Member Author

kartikdutt18 commented May 19, 2020 •

edited

Hey @saksham189, @KimSangYeon-DGU,
I have completed the implementation of NMS and added some simple tests (I will add some more) for 4 rows and n column type (i.e. each column is a separate bounding box). Kindly let me know what you think about this implementation. Reference links and test links are metioned above.
Also the build error seems unrelated.
Regards.

KimSangYeon-DGU reviewed

View reviewed changes

Member

KimSangYeon-DGU left a comment •

edited

@kartikdutt18 Awesome, I left some comments on the design and will look into this tomorrow night with the references.

src/mlpack/core/metrics/non_maximal_supression.hpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/tests/metric_test.cpp Outdated Show resolved Hide resolved

src/mlpack/tests/metric_test.cpp Outdated Show resolved Hide resolved

KimSangYeon-DGU reviewed

View reviewed changes

src/mlpack/core/metrics/non_maximal_supression.hpp Outdated Show resolved Hide resolved

kartikdutt18 force-pushed the Add-NMS branch from 78fdfdb to 3055220 Compare

May 19, 2020 16:10

kartikdutt18 commented

View reviewed changes

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

KimSangYeon-DGU removed the s: unanswered label

KimSangYeon-DGU reviewed

View reviewed changes

Member

KimSangYeon-DGU left a comment

@kartikdutt18 Thanks you for the amazing work. I left some comments on the wording and the API sides. I think we're on the last stage of this PR :)

src/mlpack/core/metrics/non_maximal_supression.hpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/tests/metric_test.cpp Show resolved Hide resolved

src/mlpack/tests/metric_test.cpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

KimSangYeon-DGU reviewed

View reviewed changes

src/mlpack/core/metrics/non_maximal_supression.hpp Outdated Show resolved Hide resolved

KimSangYeon-DGU removed the s: needs review label

KimSangYeon-DGU reviewed

View reviewed changes

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/tests/CMakeLists.txt Outdated Show resolved Hide resolved

KimSangYeon-DGU added the s: needs review label

KimSangYeon-DGU reviewed

View reviewed changes

Member

KimSangYeon-DGU left a comment

Hi @kartikdutt18. I left some additional comments for description and let's update HISTORY.md including #2402 :)

src/mlpack/core/metrics/non_maximal_supression.hpp Show resolved Hide resolved

src/mlpack/tests/metric_test.cpp Outdated Show resolved Hide resolved

src/mlpack/tests/metric_test.cpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression.hpp Outdated Show resolved Hide resolved

kartikdutt18 force-pushed the Add-NMS branch from 57ae1b9 to 4d4938d Compare

May 21, 2020 11:17

Member Author

kartikdutt18 commented May 21, 2020

Rebasing again to resolve merge conflict. All the tests have passed thanks to the recent fixes in the pipeline.

zoq reviewed

View reviewed changes

src/mlpack/core/metrics/non_maximal_supression.hpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

src/mlpack/core/metrics/non_maximal_supression_impl.hpp Outdated Show resolved Hide resolved

Member Author

kartikdutt18 commented May 21, 2020 •

edited

I think this works, it passed all the tests locally. Let me know what you think about the changes. Thanks a lot.
Regards.

Member Author

kartikdutt18 commented May 22, 2020 •

edited

Making a few more changes, Had an idea to simplify this further. Thanks.

kartikdutt18 force-pushed the Add-NMS branch 2 times, most recently from 529ba5e to 6e5f1be Compare

May 22, 2020 05:19

kartikdutt18 added 2 commits

May 22, 2020 10:50


          NMS Definition

09a22b0

Add Definition of NMS

Style Fix

Complete implementation, remove subviews next

Fix build failure to access in subview

Implementation complete for row type

Style Fix

Complete implementation, Add tests

Add tests

Return indices, style fixes, check for indices in tests as well as bounding boxes

Remove arma::reverse and use flipud

Style changes, Add param description and Update history.md

Fix two subtle bugs. :)

Update HISTORY.


          Seperate code in blocks for readibility

edf2201

Simplify code and remove any duplicacy

Better writting style

kartikdutt18 force-pushed the Add-NMS branch from 6e5f1be to edf2201 Compare

May 22, 2020 05:20

kartikdutt18 and others added 2 commits

May 22, 2020 10:51


          Merge branch 'master' into Add-NMS

1f9c950


          Style Fix

5c33887

KimSangYeon-DGU approved these changes

View reviewed changes

Member

KimSangYeon-DGU left a comment

Looks good to me 👍

kartikdutt18 added 2 commits

May 23, 2020 10:28


          Update file paths to fuller file paths as per mlpack#2400

5212f26


          Update file paths to fuller file paths as per mlpack#2400

07867c7

mlpack-bot bot approved these changes

View reviewed changes

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

mlpack-bot bot removed the s: needs review label

KimSangYeon-DGU merged commit 727345b into mlpack:master

Member

KimSangYeon-DGU commented May 23, 2020

@kartikdutt18 Thanks for the amazing work :)

Member Author

kartikdutt18 commented May 24, 2020

Thanks a lot @KimSangYeon-DGU, @zoq for the reviews and all the help.

kartikdutt18 mentioned this pull request

Some of the loss functions probably don't work correctly #2444

Closed

8 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment