Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RandAugment implementation differences. #411

Open
sebastian-sz opened this issue May 4, 2022 · 8 comments
Open

RandAugment implementation differences. #411

sebastian-sz opened this issue May 4, 2022 · 8 comments
Assignees
Labels
type:docs Improvements or additions to documentation

Comments

@sebastian-sz
Copy link
Contributor

I'm reading keras_cv RandAugment implementation and feel like there are some differences with TF implementation referenced in the paper. I'm still reading this layer so I might not have understood everything, but here it goes:

Are those differences intentional / are the missing operations not so important or should they be added in the future?

@bhack
Copy link
Contributor

bhack commented May 4, 2022

I don't know how many points was covered in private threads #84 #169

@sebastian-sz
Copy link
Contributor Author

@bhack If those differences were discussed and accepted maybe they could be described in the docstring.

For example: the Identity operation is also missing but it's mentioned that this is substituted by the rate parameter.

@bhack
Copy link
Contributor

bhack commented May 4, 2022

I've correctly updated the wrong mentioned ticket

@sebastian-sz
Copy link
Contributor Author

I see, #169 is closed but there is no mention whether it was decided to ignore or include Posterize. I still believe that a docstring mention would be good to have for such cases.

Also I can't find information/discussion regarding presence of other ops: Sharpness & Rotate.

@bhack
Copy link
Contributor

bhack commented May 4, 2022

Yes, I think also that some underground information/choices could also be linked to this kind of arguments #294 (comment)

@LukeWood
Copy link
Contributor

LukeWood commented May 5, 2022

I actually discussed this heavily with Ekin and am confident that our implementation is correct and that the others are actually NOT correct!

I can take an action item to discuss this in a "why KerasCV doc"; I am actively working with several original authors to verify our implementations of various components.

@LukeWood LukeWood self-assigned this May 5, 2022
@LukeWood LukeWood added the type:docs Improvements or additions to documentation label May 5, 2022
@sebastian-sz
Copy link
Contributor Author

@LukeWood thank you for the answer.

I did not know about internal work or discussions and was simply worried that there are quite few differences between keras_cv and other existing implementations in various libraries. If they were discussed and even approved by original authors then it sounds solid to me.

@bhack
Copy link
Contributor

bhack commented May 6, 2022

I can take an action item to discuss this in a "why KerasCV doc"; I am actively working with several original authors to verify our implementations of various components.

Also if more correct and documented I suppose that it could be still a risk for the community until we will cover our "flavour" with a full reproducibility of the train protocol.
As you know the devil is in details (and in other hyper-parameters 😄) so we need to claim and document our reproducibility to trust the community.

freedomtan pushed a commit to freedomtan/keras-cv that referenced this issue Jul 20, 2023
…-team#411)

* Fix min and max for torch when initial is not None

* Remove unnecessary if check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:docs Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants