Wrong Implementation for 'random_cutout'

[BinyanHu]

System information

• OS Platform and Distribution (e.g., Linux Ubuntu 16.04): All
• TensorFlow version and how it was installed (source or binary): 0.12.1 from source
• TensorFlow-Addons version and how it was installed (source or binary): 2.4.1 from source
• Python version: 3.8
• Is GPU used? (yes/no): yes

Describe the bug
The current implementation of 'tfa.image.random_cutout' is wrong. According to the paper Improved Regularization of Convolutional Neural Networks with Cutout, the optimal random centers of the mask patches should be uniformly sampled from the whole image, which means that the path should sometimes overlap with the image borders and be partially excluded from the image. In other words, the center x and y should be sampled from [0, length] for height and width respectively, but the current implementation is sampling from [half_size, image_length - half_size].

The current implementation is contradicted with the paper and the implementation of previous tfa versions. In practice, the current implementation also leads to decreased accuracy.

Please change the sampling back to [0, length].

[bhack]

Can you check with one of the others cutout implementations that we have in the ecosystem https://github.com/tensorflow/models/blob/master/official/vision/beta/ops/augment.py#L264-L311?

/cc @tomerk @seanpmorgan @WindQAQ We need to verify what we want to do with some of this duplicated images ops in the ecosystem

[BinyanHu]

Can you check with one of the others cutout implementations that we have in the ecosystem https://github.com/tensorflow/models/blob/master/official/vision/beta/ops/augment.py#L264-L311?

/cc @tomerk @seanpmorgan @WindQAQ We need to verify what we want to do with some of this duplicated images ops in the ecosystem

Thanks for reminding. The method in official works fine.

But as tfa.image.random_cutout is different from the paper and actually not the commonly used one, shall we add some warnings or instructions in its documentation?

[seanpmorgan]

Hi @BinyanHu thanks for reporting, we should at a minimum add warnings or instructions.

Regarding duplication in the ecosystem. I for one am exhausted at this discussion. We've spoken to the models repo several times asking them to use a modular software design and re-use code in Addons. You can not pip install the "official" augmentations in that repo. They should contribute to this repo and utilize it. cc @tomerk

I know @fchollet has echoed the sentiments around reusability.

[seanpmorgan]

@WindQAQ any good reason for this change or issue with reverting?

[WindQAQ]

The current implementation is contradicted with the paper and the implementation of previous tfa versions.

Can you provide the commit id that introduces regression?

/cc @fsx950223

[bhack]

You can not pip install the "official" augmentations in that repo. They should contribute to this repo and utilize it. cc @tomerk

I agree with you that we need to unify these ops to not double handle tickets and PR on the same stuffs in multiple repos. But in this case you can pip install those ops and layers with https://pypi.org/project/tf-models-nightly.

[seanpmorgan]

You can not pip install the "official" augmentations in that repo. They should contribute to this repo and utilize it. cc @tomerk

I agree with you that we need to unify these ops to not double handle tickets and PR on the same stuffs in multiple repos. But in this case you can pip install those ops and layers with https://pypi.org/project/tf-models-nightly.

News to me that they are releasing a python package. There's no mention of it on their repo, and the augment ops are not available for tf-models-official only nightly but the dates don't match up for it not being in their 2.4 release. I (personally) will not bother trying to coordinate with their repo anymore after our previous attempts.

[bhack]

I (personally) will not bother trying to coordinate with their repo anymore after our previous attempts.

If we cannot do better than this we could accept copy a paste PR from users when the duplicated code in the ecosystem has the correct implementation.
So in this case I suppose that we could align our implementation with https://github.com/tensorflow/models/blob/master/official/vision/beta/ops/augment.py#L264-L311.
Or fix it "clean room". But I think "clean room" implementation/fix don't make sense in our ecosystem with signed CLA.

My opinion is that if we cannot resolve duplicates we could accept @BinyanHu PR that align our own impl with official/vision.

[seanpmorgan]

Agree.. I've reached out to see if we can get an official position on this tf-model package. If there is no resolution, we should match their implementation and decide on either deprecating ours or adding them as a dependency. This is quite unfortunate. The Models repo has no public facing CI, and 1000 issue related to model details. Throwing in reusable components to that repo and packaging it all up is a mistake in my opinion.

[fsx950223]

The current implementation is contradicted with the paper and the implementation of previous tfa versions.

Can you provide the commit id that introduces regression?

/cc @fsx950223

#2285

[sayakpaul]

@BinyanHu do you mind taking a look at #2448? Would be of great help.