Skip to content

Added Kernel and Bias Initializers to Encoder and Decoder #50

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Mar 24, 2022
Merged

Added Kernel and Bias Initializers to Encoder and Decoder #50

merged 18 commits into from
Mar 24, 2022

Conversation

@aflah02
Copy link
Collaborator

@aflah02 aflah02 commented Mar 19, 2022

Fixes - #48
I've added the required parameters for the encoder and decoder and also added 2 new tests to check the config files generated. I've also added relevant docstrings in the encoder and decoder files

@aflah02 aflah02 changed the title Initializer work Added Kernel and Bias Initializers to Encoder and Decoder Mar 19, 2022
@chenmoneygithub chenmoneygithub self-assigned this Mar 19, 2022
@aflah02
Copy link
Collaborator Author

aflah02 commented Mar 21, 2022

Hey @chenmoneygithub @mattdangerw
Any reviews for this?

chenmoneygithub
chenmoneygithub reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Mainly looks good, left some comments on style.

@mattdangerw mattdangerw self-requested a review March 21, 2022 17:57
mattdangerw
mattdangerw reviewed Mar 21, 2022
View reviewed changes
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments for decoder, please mirror the changes to encoder as well. Thank you!

@aflah02
Copy link
Collaborator Author

aflah02 commented Mar 22, 2022

Hi @mattdangerw @chenmoneygithub
Thanks for the great reviews!
I've fixed almost all the points and requested clarification/approval for 2 of the points

mattdangerw
mattdangerw approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple last comments, but LGTM pending these changes.

@aflah02 aflah02 mentioned this pull request Mar 22, 2022
Closed
@mattdangerw mattdangerw mentioned this pull request Mar 22, 2022
Merged
chenmoneygithub
chenmoneygithub approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattdangerw
Copy link
Member

mattdangerw commented Mar 23, 2022

@aflah02 looks like there's some too long lines, blocking the lint check. Fix those up and we can merge. You should be able to run ./scripts/lint.sh without errors.

@aflah02
Copy link
Collaborator Author

aflah02 commented Mar 23, 2022

@mattdangerw Done, I also noticed there was a minor typo and layer was used in instead of layers in the docstring so I've fixed that as well

@mattdangerw
Copy link
Member

mattdangerw commented Mar 24, 2022

Thanks!

@mattdangerw mattdangerw merged commit ba8b150 into keras-team:master Mar 24, 2022
@aflah02 aflah02 deleted the initializerWork branch March 24, 2022 00:21
adhadse pushed a commit to adhadse/keras-nlp that referenced this pull request Sep 17, 2022
@aflah02
Added Kernel and Bias Initializers to Encoder and Decoder (keras-team#50
2f2c57a 
)

* Added Kernel and Bias Initializer to
decoder

* Added Initializer to Encoder and Decoder

* Added Initializers to Expected Test Config

* Added Serialized Version to Config, Added New Test

* Fixed Docstring for Encoder and Decoder

* Changed initializer import to keras.initializer

* Removed Redudant Test From Encoder and Decoder

* Changed Default to Glorot Uniform and Zeros

* Ensure friendly error if bad arg on layer creation

* Ran Black Formatter

* Fixed Serialization Bug and Reran Black

* Added Additional Tests for Testing Value Error

* Keeping Attribute Set. From Const. Arg. Together

* New test for Value Error if Invalid Initializer

* Ran format and lint

* Fixed typo and also lines exceeding max length
mattdangerw pushed a commit that referenced this pull request May 21, 2024
@divyashreepathihalli @mattdangerw
Add VIT Encoder (#50)
758e8ad
mattdangerw pushed a commit that referenced this pull request May 21, 2024
@divyashreepathihalli @mattdangerw
Add VIT Encoder (#50)
5083759
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattdangerw mattdangerw mattdangerw approved these changes

+1 more reviewer

@chenmoneygithub chenmoneygithub chenmoneygithub approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chenmoneygithub chenmoneygithub

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aflah02 @mattdangerw @chenmoneygithub