Skip to content

1822 Implement Prob NMS#1842

Merged
bhashemian merged 16 commits intoProject-MONAI:masterfrom
yiheng-wang-nv:1822-radial-nms
Mar 26, 2021
Merged

1822 Implement Prob NMS#1842
bhashemian merged 16 commits intoProject-MONAI:masterfrom
yiheng-wang-nv:1822-radial-nms

Conversation

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor

@yiheng-wang-nv yiheng-wang-nv commented Mar 24, 2021

Signed-off-by: Yiheng Wang [email protected]

Fixes #1822 .

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor Author

yiheng-wang-nv commented Mar 24, 2021

Hi @behxyz , could you please help to check the calculation first. I just tested locally, and will add formal test cases and doc string later.

Compared with the startup code in #1800 , this version enhances:

  1. Optimize the calculation via using more numpy based methods, now for a same 2D input probability map, the calculation speed is around 3 times faster when sigma > 0, and 5 times faster when sigma is 0.
  2. Remove skimage dependency.
  3. Supports any dimension inputs.

Copy link
Copy Markdown
Member

@bhashemian bhashemian left a comment

Choose a reason for hiding this comment

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

Well done, @yiheng-wang-nv!
Apart from some minor changes, all looks good to me. So please add docstrings and unnittests, and move on from this to implement how to actually use it in conjunction with other components during inference.

@bhashemian
Copy link
Copy Markdown
Member

@yiheng-wang-nv, maybe we should change the name from RadialNMS to PixelNMS or something. Because although we are receiving the radius as an input, we are suppressing over a square (or a cube) and not a circle (or a sphere). Unless, we use a radial base to suppress the pixels within the given radius.

@yiheng-wang-nv
Copy link
Copy Markdown
Contributor Author

@yiheng-wang-nv, maybe we should change the name from RadialNMS to PixelNMS or something. Because although we are receiving the radius as an input, we are suppressing over a square (or a cube) and not a circle (or a sphere). Unless, we use a radial base to suppress the pixels within the given radius.

Agree, do you think we should also change the name of radius?

@yiheng-wang-nv yiheng-wang-nv marked this pull request as ready for review March 25, 2021 09:45
@yiheng-wang-nv yiheng-wang-nv changed the title [WIP] 1822 Implement Radial NMS 1822 Implement Radial NMS Mar 25, 2021
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks I put some comments inline

@bhashemian
Copy link
Copy Markdown
Member

@yiheng-wang-nv
Could you please finalize this PR without resolution and add another pathology specific one with resolution in monai/apps/pathology/?

@yiheng-wang-nv yiheng-wang-nv changed the title 1822 Implement Radial NMS 1822 Implement Prob NMS Mar 26, 2021
Signed-off-by: Yiheng Wang <[email protected]>
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks!

@bhashemian bhashemian enabled auto-merge (squash) March 26, 2021 13:46
@bhashemian bhashemian merged commit b209ab7 into Project-MONAI:master Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prob Non-maximal Suppression Implementation

3 participants