2177 Add max_roi_size to RandSpatialCrop

[Nic-Ma]

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]

/black

[rijobro]

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]

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]

Thanks Nic, I added some comments for this PR.

[rijobro]

@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]

Hi @rijobro and @yiheng-wang-nv ,

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