Skip to content

add box mode convert in box_utils#4254

Merged
wyli merged 61 commits intoProject-MONAI:devfrom
Can-Zhao:detection_pr
May 18, 2022
Merged

add box mode convert in box_utils#4254
wyli merged 61 commits intoProject-MONAI:devfrom
Can-Zhao:detection_pr

Conversation

@Can-Zhao
Copy link
Copy Markdown
Collaborator

@Can-Zhao Can-Zhao commented May 11, 2022 •
edited
Loading

Signed-off-by: Can Zhao [email protected]

Fixes #3571 .

Description

Add box mode converters into box_utils

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • 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 --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Can-Zhao Can-Zhao added this to the Detection module [P0] milestone May 11, 2022
@Can-Zhao
reformat
d1424bd 
Signed-off-by: Can Zhao <[email protected]>
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented May 11, 2022

Hi @Can-Zhao ,

Thanks for putting together the first PR for the detection task.
I tried to review the PR but I feel it's too big(more than 1000 lines) to put detailed comments, could you please help split it into smaller PRs? Then we can discuss more details in the code line.
And I would suggest to add some small examples in the doc-string of several complicated box utilities?
Especially for users like me who didn't participate in your previous design & development discussions.
For example, we tried to help users understand the bundle APIs with examples:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L407
https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L37

What do you think?
CC @wyli @dongyang0122

Thanks in advance.

@Can-Zhao
Copy link
Copy Markdown
Collaborator Author

Can-Zhao commented May 11, 2022

Hi @Can-Zhao ,

Thanks for putting together the first PR for the detection task. I tried to review the PR but I feel it's too big(more than 1000 lines) to put detailed comments, could you please help split it into smaller PRs? Then we can discuss more details in the code line. And I would suggest to add some small examples in the doc-string of several complicated box utilities? Especially for users like me who didn't participate in your previous design & development discussions. For example, we tried to help users understand the bundle APIs with examples: https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L407 https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L37

What do you think? CC @wyli @dongyang0122

Thanks in advance.

Thank you so much Nic! I will split it into small PRs!

@Can-Zhao Can-Zhao changed the title add box_utils add box mode convert box_utils May 11, 2022
@Can-Zhao Can-Zhao changed the title add box mode convert box_utils add box mode convert in box_utils May 11, 2022
@Can-Zhao
clean
208f889 
Signed-off-by: Can Zhao <[email protected]>
Can-Zhao added 2 commits May 11, 2022 12:35
@Can-Zhao
correct typo
406a73c 
Signed-off-by: Can Zhao <[email protected]>
@Can-Zhao
add comments to explain box mode
6cd14ae 
Signed-off-by: Can Zhao <[email protected]>
@Can-Zhao
Copy link
Copy Markdown
Collaborator Author

Can-Zhao commented May 11, 2022

Hi @Can-Zhao ,

Thanks for putting together the first PR for the detection task. I tried to review the PR but I feel it's too big(more than 1000 lines) to put detailed comments, could you please help split it into smaller PRs? Then we can discuss more details in the code line. And I would suggest to add some small examples in the doc-string of several complicated box utilities? Especially for users like me who didn't participate in your previous design & development discussions. For example, we tried to help users understand the bundle APIs with examples: https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L407 https://github.com/Project-MONAI/MONAI/blob/dev/monai/bundle/config_parser.py#L37

What do you think? CC @wyli @dongyang0122

Thanks in advance.

Hi Nic. I think it should be ready for review. Thank you so much!

@Can-Zhao
correct typo
47b99e5 
Signed-off-by: Can Zhao <[email protected]>
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented May 12, 2022

/black

Nic-Ma
Nic-Ma reviewed May 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma left a comment •
edited by Can-Zhao
Loading

Choose a reason for hiding this comment

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

Thanks for putting together the utilities.
Put some minor comments inline.
I see several mappings of box mode and spatial dims in the code logic, like:
int(bbox.shape[1] / 2), int(len(mode) / 2)
Maybe we should define it easier and clearer to use?
For example: associate properties bounding box, mode, spatial dims, etc. as a class and define several class functions to do the mode conversion?
@wyli @dongyang0122 What do you think?

Thanks in advance.

@Can-Zhao
isort test_box_utils
5712843 
Signed-off-by: Can Zhao <[email protected]>
Nic-Ma
Nic-Ma approved these changes May 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Can-Zhao ,

Thanks for your quick update.
It overall looks good to me now, just some minor comments left:

  1. Please add your new Enum and convert_to_list to:
    https://github.com/Project-MONAI/MONAI/blob/dev/monai/utils/__init__.py
  2. Please add these new components to the docs.
  3. Ensure all the CI tests can pass.

@wyli Could you please also help take a look? Do you have any other comments?

Thanks.

@Can-Zhao
Copy link
Copy Markdown
Collaborator Author

Can-Zhao commented May 18, 2022 •
edited
Loading

Hi @Can-Zhao ,

Thanks for your quick update. It overall looks good to me now, just some minor comments left:

  1. Please add your new Enum and convert_to_list to:
    https://github.com/Project-MONAI/MONAI/blob/dev/monai/utils/__init__.py
  2. Please add these new components to the docs.
  3. Ensure all the CI tests can pass.

@wyli Could you please also help take a look? Do you have any other comments?

Thanks.

Thank you Nic.

  1. I have added Enum and convert_to_list to:
    https://github.com/Project-MONAI/MONAI/blob/dev/monai/utils/__init__.py
  2. I added convert_box_mode and convert_box_to_standard_mode to docs. I was wondering if there are more I need to add. Do I need to add the BoxMode classes in box_mode.py?
  3. The mypy test in flake8-py3 could not pass. How did you handle this case previously? Do you enforce mypy to pass?

Can-Zhao added 2 commits May 17, 2022 22:46
@Can-Zhao
reformat
da7bebe 
Signed-off-by: Can Zhao <[email protected]>
@Can-Zhao
update comment in enum
7912dfc 
Signed-off-by: Can Zhao <[email protected]>
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented May 18, 2022

Hi @Can-Zhao ,

Please add BoxMode to the docs.

Thanks.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented May 18, 2022

/black

@Can-Zhao
Copy link
Copy Markdown
Collaborator Author

Can-Zhao commented May 18, 2022

Hi @Can-Zhao ,

Please add BoxMode to the docs.

Thanks.

Done. Thanks!

wyli
wyli reviewed May 18, 2022
View reviewed changes
wyli
wyli reviewed May 18, 2022
View reviewed changes
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 docstring update suggestions

@wyli
Copy link
Copy Markdown
Contributor

wyli commented May 18, 2022

/black

@wyli
Copy link
Copy Markdown
Contributor

wyli commented May 18, 2022

/build

@wyli
fixes docstring
61dc4ab 
Signed-off-by: Wenqi Li <[email protected]>
@wyli
Copy link
Copy Markdown
Contributor

wyli commented May 18, 2022

/build

@wyli wyli enabled auto-merge (squash) May 18, 2022 17:04
@wyli
Copy link
Copy Markdown
Contributor

wyli commented May 18, 2022

/build

@wyli wyli merged commit 6eaa3ab into Project-MONAI:dev May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli left review comments

@Nic-Ma Nic-Ma Nic-Ma approved these changes

@mibaumgartner mibaumgartner Awaiting requested review from mibaumgartner

@dongyang0122 dongyang0122 Awaiting requested review from dongyang0122

Assignees

No one assigned

Labels

Feature request

Projects

None yet

Milestone

Detection module [P0]

Development

Successfully merging this pull request may close these issues.

Box utils components

4 participants

@Can-Zhao @Nic-Ma @wyli @monai-bot