Project-MONAI / MONAI

AI Toolkit for Healthcare Imaging
https://monai.io/
Apache License 2.0
5.69k stars 1.04k forks source link

RandomWeightedCrop doesn't randomly generate crops for a weightmap of integers #7949

Open gregspangenberg opened 1 month ago

gregspangenberg commented 1 month ago

Describe the bug monai.transforms.RandomWeightedCrop uses the function monai.transforms.utils.weighted_patch_samples. At line 576 in utils the weighted_patchsamples function converts the datatype of the randomly generate number in the range of (0,1) to match the datatype of the input weightmap. """ r, * = convert_to_dst_type(r_state.random(n_samples), v) """ If the weightmap consisted of integers then the random value will always be the integer of 0, which disables random sampling and instead samples from the first valid region continuously.

To Reproduce Steps to reproduce the behavior:

  1. Create a numpy weightmap with integer datatype
  2. Pass weightmap into RandomWeightedCrop
  3. Observe the "crop_center" value in the meta data of the cropped image after multiple iterations it will remain constant

Expected behavior The random factor "r" should never be an converted to an integer.

Han123su commented 1 day ago

Hi @gregspangenberg, I reviewed the code and found that when weight_map is of integer type, the random numbers should indeed not be converted to integers, as this would result in a loss of randomness. May I ask if the weight_map input has a value range if it is an integer? I haven't found any relevant documentation, but I only saw that it needs to be non-negative. If there is a value range, I plan to directly convert it to a floating point number. If not, I will consider modifying it using the method below.

I think that regardless of the data type of weight_map, random numbers should remain in float format to accurately calculate the sampling location after being multiplied by the cumulative weights. However, keeping the random numbers in float format causes issues with the searchsorted function, as it expects r and v to have matching data structures. The original convert_to_dst_type handles both data type and structure conversion, which is why there were no issues initially.

I plan to add a new option to the convert_to_dst_type to selectively convert only the data structure without changing the data type. This may also be useful in other contexts, and if implemented successfully, I will add test cases for it. Does this approach seem reasonable to you?

Expect

r, *_ = convert_to_dst_type(r_state.random(n_samples), v, structure_only=True)
def convert_to_dst_type(
    src: Any,
    dst: NdarrayTensor,
    dtype: DtypeLike | torch.dtype | None = None,
    wrap_sequence: bool = False,
    device: None | str | torch.device = None,
    safe: bool = False,
    structure_only: bool = False,    ######## Add ########
) -> tuple[NdarrayTensor, type, torch.device | None]:
...
...
if structure_only:
   ...
...