bugfix aspect ratio sampling in transforms.RandomErasing

[wbaek]

aspect ratio must be a sampling from log scale.
reference from:

vision/torchvision/transforms/transforms.py

Lines 833 to 836 in 8317295

	log_ratio = torch.log(torch.tensor(ratio))
	aspect_ratio = torch.exp(
	torch.empty(1).uniform_(log_ratio[0], log_ratio[1])
	).item()

[facebook-github-bot]

Hi @wbaek!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[datumbox]

@wbaek Thanks for the PR and welcome to our community.

I am not sure that working on log scale has any benefit. Also note that changing this would be a breaking change because sampling from a log scale uniformly is not the same as sampling uniformly on the original scale. Finally please note that our implementation is aligned with the official implementation from the paper.

Based on the above, we are not going to be able to merge this PR. I will close if to avoid accidental merges, but if you have any concerns please let me know.

[wbaek]

@datumbox

I know the aspect ratio is height / width.
So, i set ratio range 1/3 ~ 3/1 and then expected equality ratio of shape wide and narrow rectangle.
but, erased area are biased narrow rectangle on sampling from original scale. not uniform in aspect ratio.

I already checked official implementation. but it has problem too.
I know that is importan for reproduce but that is misstake. I think it is not desired.
I fixed on my code. I hoped that others would not make mistakes.
Thank.

[datumbox]

@wbaek I understand that your intention is to sample both smaller and larger ratios "equally". In effect you would like to sample from a distribution that has its middle point around 1 and covers equal areas on its left and on its right. Thus applying the logarithm transform before sampling uniformly gives you that effect.

The challenge here is that the original paper does not try to address this and its original implementation uses simply uniforms. This why I don't necessarily agree that it's a bug but a property of the proposed technique. Changing the implementation would mean introducing a breaking change to the library.

@fmassa Any thoughts on this? Perhaps we could start by showing that this bias has a detrimental effect on accuracy. If that's the case it might be worth fixing?

[fmassa]

I think this bias is not desired, and we had a similar problem in the past that we fixed, see #618 (comment) and #799 for context. So from this perspective I would be inclined to fix this in torchvision (but I agree having more data would be nice)

For reference, pytorch-image-models also fixed this bias in huggingface/pytorch-image-models@66634d2

Maybe @zhunzhong07 (one of the RandomErasing authors and who sent the original PR to torchvision adding random erasing in #909) or @rwightman have some experiments with both versions and could shed some light here?

[rwightman]

@fmassa @datumbox I fixed mine when I noticed the improvements to torchvision's random resize. I did not do extensive ablations but I have produced a lot of unmatched model weights with it in the time since.

The 'paper didn't do it' / will break compat line of reasoning shouldn't be a default fallback without further consideration. It will hurt the progress of torch/torchvision in the long run and I see it more and more. The addition of guassian noise erasing in Zhun's original PR for torchvision wasn't in the original paper / impl but due to an enhancment I proposed and ablation study I did.

[datumbox]

@rwightman Thanks for the input. I understand that this was adopted on your repo without extensive comparisons but since then you did not find any indication that it hurt model performance. As for "falling back without consideration", I hope this discussion proves this is not the case. We do need to be careful and weight things when breaking changes are introduced but if there is enough indication that a change is beneficial we are flexible.

I'm currently in the process of training a couple of additional models on our side. I'll take this patch for a test and get back to you early next week.

[zhunzhong07]

@wbaek Thanks for pointing this bias. Indeed, I set the aspect ratio with the original function of random resize.

As pointed by @fmassa and @rwightman, this issue is addressed in the new version of random resize (#799) and is fixed by @rwightman in his new version of pytroch-imagenet.

Therefore, I think this issue should be considered in random erasing. I will try to implement the results on CIFAR-10 and CIFAR-100, and to see if this change will hurt the model performance.

@datumbox Thanks for your attempts and I would like to hear your feedback on your own experiments.

[zhunzhong07]

I have run experiments on CIFAR-10, CIFAR-100, and Fashion-MNIST with ResNet-20. For each dataset, I run 5 times with different seeds (0-4). New and original implementations use the same seeds.

CIFAR-10: Original 93.41±0.11%， New 93.24±0.10%
CIFAR-100: Original 70.05±0.29%， New 70.04±0.29%
Fashion-MNIST: Original 95.38±0.04%， New 95.46±0.04%

We can find that 1) implementations of original and new achieve almost the same results on CIFAR-100 and Fashion-MNIST, 2) However, new implementation achieves slightly lower results on CIFAR-10.

I am not sure if this modification will hurt the performance of other datasets, especially ImageNet. Looking forward to your experiments @datumbox @rwightman @wbaek.

[datumbox]

@zhunzhong07 Thanks a lot for taking the time to look at this and for proving the statistics of your runs. Based on your results, this looks like it's going to be a "no-harm" change than one that improves the accuracy. I did a brief check on my side and unfortunately the variation of accuracy caused by random initialization of the weights using different seeds is quite large and thus we would need a lot more than 5 runs on ImageNet to measure the difference. This can be quite expensive and probably not worth it in this case...

@wbaek My intention due to the above is to complete a few basic quick runs on my side and merge if no issues are spotted. I left one comment on your implementation below. It's non-blocking but it would be good to fix before merging. Let me know what you think.