Correct Resize pipeline

[makecent]

Fix #209

[LeoXing1996]

Some dataset configs are influenced by this change, such as lsun-car_pad_512.
Please check and fix them as well.

[LeoXing1996]

We should consider the situation when the input scale is float or int.

[makecent]

I don't see the mmcv.imresize can take argument scale of float or int.
https://github.com/open-mmlab/mmcv/blob/51b40c332aff9d2927fcc252b248d295850a4d55/mmcv/image/geometric.py#L51-L95

[makecent]

I think the original Resize pipeline in the mmgen does NOT support taking as input scale of float or int. I tried and got an error raised.

[LeoXing1996]

However, mmcv.imrescale can take float as input. In MMGen's Resize, we compose mmcv.imrescale and mmcv.imresize in one function. Refers to

mmgeneration/mmgen/datasets/pipelines/augmentation.py

Lines 162 to 177 in 69333cf

	if self.keep_ratio:
	img, scale_factor = mmcv.imrescale(
	img,
	scale,
	return_scale=True,
	interpolation=self.interpolation,
	backend=self.backend)
	else:
	img, w_scale, h_scale = mmcv.imresize(
	img,
	scale,
	return_scale=True,
	interpolation=self.interpolation,
	backend=self.backend)
	scale_factor = np.array((w_scale, h_scale), dtype=np.float32)
	return img, scale_factor

[LeoXing1996]

You may use the following command to run the unit test locally.

 coverage run --branch --source mmgen -m pytest -s tests/test_datasets/test_pipelines/test_augmentation.py

[makecent]

I see... I think it's a little bit confusing though. Current Resize:

Cases	Arguments	Comments
scale with a factor & keep ratio	scale=float, keep_ratio=True	lack of asserting keep_ratio=True; not support scale=int
scale with factors (fh, fw) & not keep ratio	not supported
scale with size (h, w) & not keep ratio	scale=(h, w), keep_ratio=False	Misplaced h and w
scale with size (h, -1) & keep ratio	scale=(h, -1), keep_ratio=True;	lack of asserting keep_ratio=True; variable max_long_edge could be the actual short edge

[makecent]

I can help resolve the problems:

1. lacking asserting keep_ratio=True;
2. not support scale=int;
3. Misplaced h and w (solved).

But as for the " variable max_long_edge could be the actual short edge", it requires futher discussion.

[LeoXing1996]

Your analysis of cases 1 and 2 are correct. And for case 3 we automatically rescale short edge and this behavior is independent of the position of -1 in scale.

[makecent]

@LeoXing1996 I cannot understand the codes below when there is a -1 in the scale:

mmgeneration/mmgen/datasets/pipelines/augmentation.py

Lines 134 to 139 in 69333cf

	elif mmcv.is_tuple_of(scale, int):
	max_long_edge = max(scale)
	max_short_edge = min(scale)
	if max_short_edge == -1:
	# assign np.inf to long edge for rescaling short edge later.
	scale = (np.inf, max_long_edge)

mmgeneration/mmgen/datasets/pipelines/augmentation.py

Lines 199 to 205 in 69333cf

	elif isinstance(self.scale, tuple) and (np.inf in self.scale):
	# find inf in self.scale, calculate ``scale`` manually
	h, w = results[self.keys[0]].shape[:2]
	if h < w:
	scale = (int(self.scale[-1] / h * w), self.scale[-1])
	else:
	scale = (self.scale[-1], int(self.scale[-1] / w * h))

Why don't just simply replace the -1 with a computed size that ensures keeping the ratio, but comparing height and width? My understanding is that you want to only resize the short edge no matter it's the height or width. But this is would be very confusing because:
(1) In this case, there will be NO difference between the scale=(-1, 256) and scale=(256, -1).
(2) As the experience of using numpy and torch, -1 represents that the length at this specific dimension is automatically computed.
I think it would be better to set a new argument for resizing short edges.

Anyway, it should be new topic and this pull request only resolves the "misplaced h and w" and it should work correctly now. Pls check it.

[LeoXing1996]

Yes, in the current code, the output of scale=(-1, 256) is the same as scale=(256, -1), and this means rescaling the shortest edge of the input to 256.
Your advice about automatically computing the length of the -1's edge is fair enough. You can open another PR to support this feature.

[makecent]

Yes, in the current code, the output of scale=(-1, 256) is the same as scale=(256, -1), and this means rescaling the shortest edge of the input to 256. Your advice about automatically computing the length of the -1's edge is fair enough. You can open another PR to support this feature.

I am happy that you agree with me. But changing the default operation could be dangerous, and I found that other mmlab repositories, e.g. mmaction2, also use -1 to represent the resizing on short edges. It would be better if your mmlab can get consistent on this. And after that I am willing to help.

[LeoXing1996]

Yes, in the current code, the output of scale=(-1, 256) is the same as scale=(256, -1), and this means rescaling the shortest edge of the input to 256. Your advice about automatically computing the length of the -1's edge is fair enough. You can open another PR to support this feature.

I am happy that you agree with me. But changing the default operation could be dangerous, and I found that other mmlab repositories, e.g. mmaction2, also use -1 to represent the resizing on short edges. It would be better if your mmlab can get consistent on this. And after that I am willing to help.

You are right, openmmlab's repos treat -1 as short edges resize. Therefore I think you can open another issue and PR about how we handle -1 in scale. And in this PR, we only focus on bugs of case 1 and case 2.

[makecent]

Yes, in the current code, the output of scale=(-1, 256) is the same as scale=(256, -1), and this means rescaling the shortest edge of the input to 256. Your advice about automatically computing the length of the -1's edge is fair enough. You can open another PR to support this feature.

I am happy that you agree with me. But changing the default operation could be dangerous, and I found that other mmlab repositories, e.g. mmaction2, also use -1 to represent the resizing on short edges. It would be better if your mmlab can get consistent on this. And after that I am willing to help.

You are right, openmmlab's repos treat -1 as short edges resize. Therefore I think you can open another issue and PR about how we handle -1 in scale. And in this PR, we only focus on bugs of case 1 and case 2.

I solved them in the latest commit, pls check if current version is ok.

[LeoXing1996]

You should run pre-commit install before committing to avoid lint errors.

[LeoXing1996]

When -1 in the given scale, we manually calculate the size of image. Therefore we should use mmcv.imresize and keep_ratio should not be True. I wonder if this can pass the unit test.

[makecent]

When -1 in the given scale, we manually calculate the size of image. Therefore we should use mmcv.imresize and keep_ratio should not be True. I wonder if this can pass the unit test.

My bad, forget to do the test. But don't you think that here keep_ratio as true is logically correct, since the size is computed according to the ratio?

[LeoXing1996]

Maybe setting keep_ratio as True is logically correct because we calculate the image size manually. However, in the current code, we would call mmcv.imrescale when keep_ratio is True. I suggest in this PR, we only fix the bug of size misplace and we can try to refactor the Resize class later.

[LeoXing1996]

I think you should convert assert keep_ratio to assert not keep_ratio.

[makecent]

I rewrited the if-else sentence. But it still cannot pass the pytest because the pytest also need to be updated. Here comes a new question -- when the scale is of float/int or there is a -1 in scale, should we set the keep_ratio to True by default or raise an AssertationError?

[LeoXing1996]

I rewrited the if-else sentence. But it still cannot pass the pytest because the pytest also need to be updated. Here comes a new question -- when the scale is of float/int or there is a -1 in scale, should we set the keep_ratio to True by default or raise an AssertationError?

When scale is float or int, we should assert keep_ratio is True.
When -1 in scale and scale is a list, keep_ratio should not be True, because we want to call mmcv.imresize.