cropforeground with margin>0

[wyli]

Describe the bug

import numpy as np
import monai.transforms as mt
from monai.visualize import matshow3d

mask_gt = np.zeros((1, 100, 100), np.uint8)
mask_gt[0, 0:50] = 1

output = mt.CropForeground(margin=2)(mask_gt)
print("cropped size: ", output.shape)
matshow3d(output, show=True)

should crop and add a 2pixel margin to each side of the output because margin=2.
but the result with the latest dev branch (up to e7fb74f)is:

In my understanding, the expected output would be:

[KumoLiu]

Hi @wyli, It looks like the error comes from line 1001, if allow_smaller, it will pad the output.

MONAI/monai/transforms/utils.py

Lines 1001 to 1003 in e7fb74f

	if allow_smaller:
	min_d = max(min_d, 0)
	max_d = min(max_d, spatial_size[di])

I will create a PR to fix it.

[myron]

@wyli @KumoLiu

I'm a little confused with the change here,..

in my understanding CropForeground(margin=2) with allow_smaller=True (which was the default until this commit) was already doing the right thing.

we usually want to crop image around the tumor (or around visible foreground). So If I want to crop around tumor with margin 10, I expect it to find a tight box around it, add 10px (in all directions) , but if anything is outside of the image, i don't need any padding with zeros. I only want the image content around tumor and a margin 10 (but only when possible). So CropForeground(margin=10, allow_smaller=True) was previously doing that (I think). which is to allow the region to be smaller (adjust to image bounds, without any padding)
Now the meaning of allow_smaller is the reverse, and in the new change, it will add a border with zeros around the tumor if it's outside the image content. and to get the previous result, I need to manually change to allow_smaller=False everywhere..

is my understanding correct here? or am I confused why we're changing it? thank you

[KumoLiu]

Hi @myron, let me clarify the change for you.
First, let's examine the current behavior when margin is set to 2. After cropping the foreground, if the resulting box edges extend beyond the image boundaries, by default, no padding is applied unless allow_smaller is set to True. The key consideration here is determining what is considered "smaller." In this context, we assume that setting allow_smaller to True permits images smaller than the final box size, in which case padding will be applied regardless.

Let me provide a simple example to illustrate this. In the given scenario, the output will be padded by default. However, if you modify mask_gt[0, 0:50, 25:50]=1, it will leave one edge without padding.

import numpy as np
import monai.transforms as mt
from monai.visualize import matshow3d

mask_gt = np.zeros((1, 100, 100), np.uint8)
mask_gt[0, 25:50, 25:50] = 1

output = mt.CropForeground(margin=2)(mask_gt)
print("cropped size: ", output.shape) -- > # torch.Size([1, 29, 29])
matshow3d(output, show=True)

I'm not sure whether you mean we need to change the default value of the allow_smaller to True, then it will keep padding regardless of whether the final size exceeds the boundaries of the image.

Hope I have explained it clearly, thanks!

[myron]

Hi @KumoLiu , so you've changed what "allow_smaller" parameter means now (regardless if it is more intuitive or not, it changes the code that used that parameter before).

• For instance, we use CropForegroundd here

CropForegroundd(keys=keys, source_key=self.image_key, allow_missing_keys=True, margin=10, allow_smaller=True)

https://github.com/Project-MONAI/research-contributions/blob/main/auto3dseg/algorithm_templates/segresnet/scripts/segmenter.py#L219--L225

The behavior of that transform has now changed after your commit, and a user will not get the same result. If other users used allow_smaller=True in their code, then their code will not work as before too to allow_smaller=False.

https://github.com/Project-MONAI/MONAI/pull/6686/files#diff-96cd15307195a8d855e3fb33c35abaa18afaecc182874751af4eb3144726d158R1002-R1004

2. Secondly I though the meaning of "allow_smaller" was already correct , so in @wyli example above it produced correct output, the margin should not be added outside the image (if allow_smaller is True, which as a default)

3. Finally, I noticed the change because after your commit - (randomly) I get my 3D CT images of complete Zeros (empty)

The transforms I have are

- CropForegroundd( keys=['image', 'label'], source_key='label',  margin=64, allow_smaller=True) #crop to label (not to image)
- SpatialPadd(keys=['image', 'label'], spatial_size=[256,256,256])
- RandCropByLabelClassesd(keys=['image', 'label'], spatial_size=[256,256,256])

Both image and label are returned of all zeros, but of correct shape [1, 256,256,256]. It happens randomly, (usually for several images in the dataset, always different images). The all Zero output is after the "RandCropByLabelClassesd" transform, not after "CropForegroundd". And I could not figure it out, so far I only narrow it down to your exact commit. Before it works fine, after I get these random "zeros".. Since you've changed what allow_smaller means, may be it has to with it somehow..

[wyli]

So the original bug I'm reporting is for the case of CropForeground(margin=2): I think without any additional input arguments, we'd expect that all the output windows would have been padded with 2-voxel margins for all spatial axes in both directions. The previous implementation and docstring were unclear which was causing these confusion. So we don't add a deprecation warn to add more confusions and instead handled it as a bug. But since this issue is raised again, perhaps we can update the docstring to clarify that the behaviour has been corrected after 1.2. Cc @Nic-Ma @ericspod

[myron]

I think there is a different way to achieve this, but not as in this PR. It's okay to change the default value of "allow_smaller", but this PR also changed what "allow_smaller" represents, so even when manually setting it to allow_smaller=True, we can't achieve the same result.

what previously was CropForeground(margin=64, allow_smaller=True) is not the same any more. Instead of tightly cropping around tumor (with potential margin within image bounds), we can get a large boundary of zeros. so, we'll need to update existing code in monaicontributions that uses it and retest, and monai users will have to do the same if they have it in their code.

On a side note - I also didn't think it was a bug in your example, it behaved according to my expectation from the docs. We usually need to crop around an organ or tumor, without extending outside the image content. the only time I need to pad outside the image is when I specify a minimal ROI (but that can't achieved with this transform anyway)

perhaps there is a different way to achieve it without breaking the existing logic. and if not, then we will need to solve a new bug, as I'm getting all zero outputs now.

[KumoLiu]

Hi @myron, I agree that my PR changed the behavior without providing a deprecation warning. As @wyli mentioned, we may need to update the docstring to clarify. However, I did not change the default behavior in this PR; you can simply try @wyli's example above, which produces the same result. You can also only change 'allow_smaller' to get the same result as before.

For the third question you mentioned, could you please share the data? Maybe I can help take a look at it.

[wyli]

My use case was to crop the largest foreground to make the surface distance computation more efficient

MONAI/monai/metrics/utils.py

Lines 178 to 182 in 4afa2ad

	cropper = CropForegroundD(
	["pred", "gt"], source_key="src", margin=1, allow_smaller=True, start_coord_key=None, end_coord_key=None
	)
	cropped = cropper({"pred": seg_pred, "gt": seg_gt, "src": or_vol}) # type: ignore
	seg_pred, seg_gt = cropped["pred"][0], cropped["gt"][0]

if the expected margin doesn't exist in some cases, it'll cause some strange bugs when doing the convolution encoding

MONAI/monai/metrics/utils.py

Lines 191 to 193 in 4afa2ad

	conv = torch.nn.functional.conv3d if spatial_dims == 3 else torch.nn.functional.conv2d
	vol = torch.stack([seg_pred[None], seg_gt[None]], dim=0).float() # type: ignore
	code_pred, code_gt = conv(vol, k.to(vol))

[myron]

I understand your example, thank you, It will probably also benefit from a new parameter "minimal_shape", so that the cropped region should never be smaller then some minimal roi (I'd like to have that feature too).

but anyway this PR looks small, but introduced a breaking change, suddenly my experiments got lower numbers, and a strange bug appeared, and I spent a good part of the day narrowing it down to this PR :( I think other users might get the same experience, but they won't have the knowledge to debug to what happened.

[myron]

may we can have CropForeground(margin=64) behave the new way (with allow_smaller=False new default) but keep the CropForeground(margin=64, allow_smaller=True) - behaving the same as it was prior to his PR?

[wyli]

but anyway this PR looks small, but introduced a breaking change, suddenly my experiments got lower numbers, and a strange bug appeared, and I spent a good part of the day narrowing it down to this PR :( I think other users might get the same experience, but they won't have the knowledge to debug to what happened.

may we can have CropForeground(margin=64) behave the new way (with allow_smaller=False new default) but keep the CropForeground(margin=64, allow_smaller=True) - behaving the same as it was prior to his PR?

thanks, that's a good point. agree with this solution, maybe we can further warn the default value change when allow_smaller is unspecified (the user is relying on the default behaviour). what do you think @KumoLiu?

[KumoLiu]

Thanks, @wyli and @myron, I just create this PR to avoid changing the semantics of allow_smaller. I also revert the default value of allow_smaller and add a deprecated_arg_default to change the default value after 1.3 if you think it's necessary.

[myron]

Old issue, but still, I'm getting so many warnings when using this transforms

CropForegroundd(keys=keys, source_key='label', allow_smaller=True)

I'm not leaving anything to defaults, and always specify the parameter "allow_smaller=True"
then is on 8 gpu machine, this is the output below.
Is it expected to see so many warnings? and any chance we can remove them?
I suppose the warning should be shown only when the value was not specified at all. Or you can just remove the warning completely, until you're ready to make the change in 1.3 . thanks you so much!

/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
  warn_deprecated(argname, msg, warning_category)

/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
warn_deprecated(argname, msg, warning_category)

/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
warn_deprecated(argname, msg, warning_category)

/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
  warn_deprecated(argname, msg, warning_category)

/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
  warn_deprecated(argname, msg, warning_category)

/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
  warn_deprecated(argname, msg, warning_category)

/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
  warn_deprecated(argname, msg, warning_category)
/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
  warn_deprecated(argname, msg, warning_category)
/usr/local/lib/python3.10/dist-packages/monai/utils/deprecate_utils.py:321: FutureWarning: 
monai.transforms.croppad.array CropForeground.__init__:allow_smaller: Current default value of argument 
`allow_smaller=True` has been deprecated since version 1.2. It will be changed to `allow_smaller=False` in version 1.3.
  warn_deprecated(argname, msg, warning_category)

[wyli]

hi @myron, yes the warning is only displayed if there's code depending on the default value. so it comes from somewhere CropForeground is used without specifying allow_smaller.

e.g.

MONAI/monai/auto3dseg/utils.py

Line 65 in 3b27bb6

copper = CropForeground(select_fn=lambda x: x > 0)

[myron]

I see, you're right. There are calls to it in DataAnalyzer() without the parameter allow_smaller... We probably need to update all calls to CropForeground() to CropForeground(allow_smaller=True) in monai codebase, if you're going to change the default value of allow_smaller in the future. Otherwise our previous results, won't be reproducible.