Skip to content

2177 Add max_roi_size to RandSpatialCrop#2178

Merged
Nic-Ma merged 8 commits intoProject-MONAI:devfrom
Nic-Ma:2177-add-max-roi
May 12, 2021
Merged

2177 Add max_roi_size to RandSpatialCrop#2178
Nic-Ma merged 8 commits intoProject-MONAI:devfrom
Nic-Ma:2177-add-max-roi

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented May 12, 2021 •
edited
Loading

Fixes #2177 .

Description

This PR added support to limit the max random crop size in RandSpatialCrop.

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.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented May 12, 2021

/black

@Nic-Ma Nic-Ma marked this pull request as ready for review May 12, 2021 07:57
@Nic-Ma Nic-Ma changed the title [WIP] 2177 Add max_roi_size to RandSpatialCrop 2177 Add max_roi_size to RandSpatialCrop May 12, 2021
@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented May 12, 2021

Do you think it would look nicer if, instead of adding max_roi_size, we modified roi_size to also take Sequence[Sequence[int]] i.e., roi_size=((10,10,10), (20, 20, 20))? It's just a shame that we have roi_size and max_roi_size, where roi_size actually means min_roi_size.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented May 12, 2021 •
edited
Loading

Hi @rijobro ,

Thanks for your suggestion, but the problem is that:

  1. roi_size only acts as "min" size when random_size=True, if False, it's the fixed roi size to crop.
  2. We also support to set only 1 value for all the spatial dims in roi_size, for example: roi_size=128, if we put max_roi_size input roi_size as you suggested, when crop a 2D image, what roi_size=(128, 192) means? It can be roi_size=(128, 128) and max_roi_size=(192, 192), or roi_size=(128, 192), right?

What do you think?

Thanks.

yiheng-wang-nv
yiheng-wang-nv reviewed May 12, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@yiheng-wang-nv yiheng-wang-nv left a comment

Choose a reason for hiding this comment

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

Thanks Nic, I added some comments for this PR.

@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented May 12, 2021

@Nic-Ma Ok makes sense. I suppose we can't change the behaviour of roi_size either, because of backwards compatibility. If we were designing it now, I would have roi_size, min_roi_size and max_roi_size where you can specify the first one or the latter two. Ah well, looks good!

I'll approve but agree with @yiheng-wang-nv 's suggested changes.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented May 12, 2021

Hi @rijobro and @yiheng-wang-nv ,

Thanks for your review, I will try to update the PR to address all the comments.

@Nic-Ma Nic-Ma enabled auto-merge (squash) May 12, 2021 12:38
@Nic-Ma Nic-Ma merged commit 89e3ff3 into Project-MONAI:dev May 12, 2021
yanielc pushed a commit to yanielc/MONAI that referenced this pull request May 13, 2021
@Nic-Ma @yanielc
2177 Add max_roi_size to RandSpatialCrop (Project-MONAI#2178)
6ae2ab2 
* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] optimize logic

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] update according to comments

Signed-off-by: Nic Ma <[email protected]>
Signed-off-by: Yaniel Cabrera <[email protected]>
yanielc pushed a commit to yanielc/MONAI that referenced this pull request May 13, 2021
@Nic-Ma @yanielc
2177 Add max_roi_size to RandSpatialCrop (Project-MONAI#2178)
f04c3cc 
* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] optimize logic

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] update according to comments

Signed-off-by: Nic Ma <[email protected]>
Signed-off-by: Yaniel Cabrera <[email protected]>
wyli pushed a commit that referenced this pull request May 26, 2021
@Nic-Ma @wyli
2177 Add max_roi_size to RandSpatialCrop (#2178)
e129152 
* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] optimize logic

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] update according to comments

Signed-off-by: Nic Ma <[email protected]>
wyli pushed a commit that referenced this pull request May 26, 2021
@Nic-Ma @wyli
2177 Add max_roi_size to RandSpatialCrop (#2178)
d6634db 
* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] optimize logic

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] update according to comments

Signed-off-by: Nic Ma <[email protected]>
wyli pushed a commit that referenced this pull request May 26, 2021
@Nic-Ma @wyli
2177 Add max_roi_size to RandSpatialCrop (#2178)
1d85674 
* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] optimize logic

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] update according to comments

Signed-off-by: Nic Ma <[email protected]>
wyli pushed a commit that referenced this pull request May 27, 2021
@Nic-Ma @wyli
2177 Add max_roi_size to RandSpatialCrop (#2178)
aefa54d 
* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add max_roi_size

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] optimize logic

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] update according to comments

Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma Nic-Ma deleted the 2177-add-max-roi branch July 2, 2021 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yiheng-wang-nv yiheng-wang-nv yiheng-wang-nv left review comments

@rijobro rijobro rijobro approved these changes

@wyli wyli Awaiting requested review from wyli

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add max_roi_size support in RandSpatialCrop

3 participants

@Nic-Ma @rijobro @yiheng-wang-nv