RandCropByLabelClasses, internal variable "ratios" is mutated

[myron]

In the cropping transform RandCropByLabelClasses, if a user specified "ratios" (a probability of sampling from each class), that variable mutated during training, on this line

MONAI/monai/transforms/utils.py

Lines 570 to 574 in ab800d8

	for i, array in enumerate(indices):
	if len(array) == 0:
	warnings.warn(f"no available indices of class {i} to crop, set the crop ratio of this class to zero.")
	ratios_[i] = 0

that means, that during training, if some classes are missing , the probability is set to 0, and it's 0 for all next images too..

here ratios_[i] is actually 'self.ratios', so we're updating the internal variable , which we should not touch.

This is the second time (recently), that we detect a internal variable being mutated (e.g. here for pixdim #5950 (comment)). These bugs are very subtle, often don't trigger any errors, just a wrong performance.

Ideally, we need to run some test on all "transforms" and ensure that no internal variables are modified/mutated. Is there an automated analysis tool for this? maybe mypy with some option?

I think, we also should not use List for internal variables (as here) or np.array (as for pixdim), and always use some immutable type (e.g. tuple, frozenlist)..

[wyli]

that's a good point, I'll review the crop/pad transforms. @yiheng-wang-nv @KumoLiu could you please help review the spatial transforms? for example

the user-specified properties such as self.spatial_axes

MONAI/monai/transforms/spatial/array.py

Lines 1220 to 1221 in b85e2c6

	self.max_k = max_k
	self.spatial_axes = spatial_axes

shouldn't change during self.__call__

but the randomize method changes internal properties such as self. _rand_k should be allowed:

MONAI/monai/transforms/spatial/array.py

Lines 1225 to 1229 in b85e2c6

	def randomize(self, data: Any | None = None) -> None:
	super().randomize(None)
	if not self._do_transform:
	return None
	self._rand_k = self.R.randint(self.max_k) + 1

[myron]

I don't think this bug is fixed.

the variable self.ratios is still a "list" (not tuple) and is being changed here

MONAI/monai/transforms/utils.py

Line 573 in a8302ec

ratios_[i] = 0

the variable "ratios_[i]", comes from self.ratios (which is also a list and is being mutated)

MONAI/monai/transforms/croppad/array.py

Line 1208 in a8302ec

self.centers = generate_label_classes_crop_centers(

I though we've agreed to change it to "tuple", but I don't see this. I see many other changes in #6127, but nothing related to this particular bug. Please correct me if I'm wrong. @wyli

[KumoLiu]

Hi @myron, I think the bug is fixed by Line 563, it generates a new list instead of using the original one. And maybe ratios_ here can't be a "tuple" since it needs to be updated here.
https://github.com/wyli/MONAI/blob/0d26394e675a574d939e15388fac5c28bbfd833e/monai/transforms/utils.py#L563

from monai.utils import ensure_tuple

ratios = [0.4, 0.5, 1.0]
ratios_previous = ([1] * len(indices)) if ratios is None else ratios
ratios_previous[1] = 0
print(ratios)  --> [0.4, 0, 1.0]
print(id(ratios) == id(ratios_previous)) --> True

ratios = [0.4, 0.5, 1.0]
ratios_new = list(ensure_tuple([1] * len(indices) if ratios is None else ratios))
ratios_new[1] = 0
print(ratios)  --> [0.4, 0.5, 1.0]
print(id(ratios) == id(ratios_new)) --> False

Please correct me if I'm wrong. :) @wyli
Thanks!

[myron]

@KumoLiu , I see, you're right, the ratios_ is converted to tuple and then to list again, so it won't change the self.ratios. all good, thank you

I still think we need to make self.ratios of "class RandCropByLabelClasses" to be a "tuple" or converted to tuple in init() - to make it more robust. Otherwise somebody else may accidentally change self.ratios in the future, and we'll be chasing a similar bug again.

[wyli]

I still think we need to make self.ratios of "class RandCropByLabelClasses" to be a "tuple" or converted to tuple in init() - to make it more robust. Otherwise somebody else may accidentally change self.ratios in the future, and we'll be chasing a similar bug again.

I included a test case in the PR using tuple as input https://github.com/Project-MONAI/MONAI/pull/6127/files#diff-18bbbf3f78e0ba08f6ff6a953e6ff5eef763ace5e1620814e940d4d35590b207

it'll raise an error if we try to make in-place changes to self.ratios

[wyli]

I didn't change any of the transform constructors though, it might be useful to add ensure_tuple to all the relevant variables in __init__(...)