search

tensorflow / addons

Useful extra functionality for TensorFlow 2.x maintained by SIG-addons
Apache License 2.0
1.69k stars 611 forks source link

Wrong Implementation for 'random_cutout' #2384

Closed BinyanHu closed 1 year ago

BinyanHu commented 3 years ago

System information

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

WindQAQ commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

https://github.com/tensorflow/addons/pull/2285

sayakpaul commented 3 years ago

@BinyanHu do you mind taking a look at https://github.com/tensorflow/addons/issues/2448? Would be of great help.

fsx950223 commented 3 years ago

So should I revert #2285 or wait for sync?

seanpmorgan commented 1 year ago

TensorFlow Addons is transitioning to a minimal maintenance and release mode. New features will not be added to this repository. For more information, please see our public messaging on this decision: TensorFlow Addons Wind Down

Please consider sending feature requests / contributions to other repositories in the TF community with a similar charters to TFA: Keras Keras-CV Keras-NLP