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.

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

Rescaling Preprocessing Layer #6840

Merged
merged 8 commits into from Sep 22, 2022
Merged

Rescaling Preprocessing Layer #6840

merged 8 commits into from Sep 22, 2022

Conversation

AdamLang96
Copy link
Contributor

@AdamLang96 AdamLang96 commented Sep 16, 2022 •
edited by nsthorat

Co-authored-by:
David Kim (@koyykdy) dok098@ucsd.edu
Brian Zheng (@Brianzheng123) brianzheng345@gmail.com

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

This change is Reviewable

@AdamLang96
Rescaling Preprocessing Layer
9ecec05 
Co-authored-by:
David Kim (@koyykdy) <dok098@ucsd.edu>
Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com>
@AdamLang96
Merge pull request #1 from AdamLang96/master
ae5f1df 
Rescaling Preprocessing Layer
mattsoulanille
mattsoulanille requested changes Sep 17, 2022
View reviewed changes
Copy link
Member

@mattsoulanille mattsoulanille 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 @AdamLang96! I only have a few small code changes for you to make before I'll approve this PR.

On the legal side of things, since this is CodeSmith's first PR to TFJS, CodeSmith will likely need to sign Googles Corporate CLA before this PR can be merged (I'm just checking with our lawyers to make sure of this).

tfjs-layers/src/exports_layers.ts Outdated Show resolved Hide resolved
tfjs-layers/src/layers/preprocessing/image_preprocessing.ts Outdated Show resolved Hide resolved
tfjs-layers/src/layers/preprocessing/image_preprocessing.ts Outdated Show resolved Hide resolved
tfjs-layers/src/layers/preprocessing/image_preprocessing.ts Outdated Show resolved Hide resolved
tfjs-layers/src/layers/preprocessing/image_preprocessing_test.ts Outdated Show resolved Hide resolved
tfjs-layers/src/layers/preprocessing/image_preprocessing_test.ts Show resolved Hide resolved
tfjs-layers/src/exports_layers.ts Outdated Show resolved Hide resolved
tfjs-layers/src/layers/preprocessing/image_preprocessing.ts Show resolved Hide resolved
@mattsoulanille
Copy link
Member

Looks like this is failing CI due to tslint. You can run the linter locally by running yarn lint in the root of the repository.

koyykdy and others added 4 commits September 19, 2022 09:59
@koyykdy
PR issues resolved
585c062
@koyykdy
linting and PR issues resolved
3f6cff6 
Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com>
Co-authored-by: (@Brianzheng123) <brianzheng345@gmail.com>
@AdamLang96
Merge pull request #2 from koyykdy/davidkim
f3f528e 
Linting and PR issues resolved
@koyykdy
Merge pull request #3 from CodeSmithDSMLProjects/davidkim
20ffb7b 
Linting updates checked over, approved
@AdamLang96
Copy link
Contributor Author

@mattsoulanille We have made the requested changes and signed a CLA agreement on behalf of Codesmith. Please let me know if there is anything else we can do. Thanks!

mattsoulanille
mattsoulanille reviewed Sep 20, 2022
View reviewed changes
Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

@AdamLang96 LGTM on the code side of things. Thanks for making the formatting and testing changes.

One last question on the legal side of things. The corporate CLA that CodeSmith signed was signed by someone with the title "Engineering Fellow." Are they authorized to sign on behalf of CodeSmith?

@AdamLang96
Copy link
Contributor Author

@AdamLang96 LGTM on the code side of things. Thanks for making the formatting and testing changes.

One last question on the legal side of things. The corporate CLA that CodeSmith signed was signed by someone with the title "Engineering Fellow." Are they authorized to sign on behalf of CodeSmith?

Just to make sure I had Alex Zai, the co-founder of Codesmith, resubmit the CLA. There should be two on file now so I apologize for the confusion.

@mattsoulanille
Copy link
Member

mattsoulanille commented Sep 21, 2022 •
edited

Thanks! We received the signature on our side. I'll merge this PR now edit: once another team member takes a look, and future ones should be a lot easier.

Linchenn
Linchenn approved these changes Sep 21, 2022
View reviewed changes
Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for implementing it!

@AdamLang96
Copy link
Contributor Author

@mattsoulanille @Linchenn I was wondering if it is possible to run a single unit test within tfjs-layers. As of right now i am running yarn test in tfjs-layers which runs all unit tests and havent figured out how to run a specific _test.ts file. Thanks!

@Linchenn
Copy link
Collaborator

Could you try grep? For example, use yarn test --//:grep='linear activation' in tfjs-layers to test https://github.com/tensorflow/tfjs/blob/master/tfjs-layers/src/activations_test.ts#L19

@AdamLang96
Copy link
Contributor Author

@Linchenn Yes that works! Thank you!!

@mattsoulanille
Copy link
Member

@AdamLang96, It looks like this branch is out-of-date with the base tfjs branch. If you allow edits from maintainers, we can update the branch and get it merged. Alternatively, you can update the branch and ping me, but it might take a few rounds since we don't have a commit queue yet.

@AdamLang96
Copy link
Contributor Author

@mattsoulanille I am updating the branch as we are having some issues with github allowing edits from maintainers (I think this is due to us making a PR from an organization's fork and not a personal fork). We have invited you as a contributor to our repo which will hopefully fix the problem.

@mattsoulanille
Copy link
Member

@AdamLang96 Thanks! Invitation accepted. I'll merge this once CI passes (and update the branch again if necessary).

@mattsoulanille mattsoulanille merged commit cce7d9f into tensorflow:master Sep 22, 2022
@mattsoulanille mattsoulanille mentioned this pull request Sep 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsoulanille mattsoulanille mattsoulanille approved these changes

@Linchenn Linchenn Linchenn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AdamLang96 @mattsoulanille @Linchenn @koyykdy