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

Category Encoding Layer #6855

Merged

Conversation

AdamLang96
Copy link
Contributor

@AdamLang96 AdamLang96 commented Sep 26, 2022

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


This change is Reviewable

* Category Encoding Preprocessing Layer
Co-authored-by:
David Kim (@koyykdy) <dok098@ucsd.edu>
Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com>
@AdamLang96 AdamLang96 changed the title Category Encoding Layer (#24) Category Encoding Layer Sep 26, 2022
@AdamLang96
Copy link
Contributor Author

@mattsoulanille I was hoping to get a review of this whenever you are available to. Thank you!

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! I have a few changes for you to take a look at. Feel free to @ me immediately in future PRs, or when you're ready for me to take another look, and I'll respond sooner.

@AdamLang96
Copy link
Contributor Author

@mattsoulanille Thank you for the review! I'll ping you once these changes have been implemented

AdamLang96 and others added 18 commits September 30, 2022 12:53
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
* revision for catencoding pr

* fixed tests and reviewer reccomendations

* fixed changed files for clean pr
* revision for catencoding pr

* fixed tests and reviewer reccomendations

* fixed changed files for clean pr

* fixed indentation and linting error

* fixed return statement in preprocessing function
@AdamLang96
Copy link
Contributor Author

AdamLang96 commented Oct 3, 2022

Hi @mattsoulanille I made the requested changes and checked that the linter was passing via yarn lint however I am failing the CI build. Do you have any insight on why this could be happening?

@mattsoulanille
Copy link
Member

mattsoulanille commented Oct 3, 2022

Looks like it's failing one of the bounds tests:

Chrome 105.0.0.0 (Mac OS 10.13.6) Category Encoding webgl2 {"WEBGL_VERSION":2,"WEBGL_CPU_FORWARD":false,"WEBGL_SIZE_UPLOAD_UNIFORM":0} Raises Value Error if max input value !<= numTokens FAILED
	Expected function to throw an exception with message 'Input values must be between 0 < values <= numTokens
	        with numTokens=3', but it threw an exception with message 'Input values must be between 0 < values <= numTokens
	          with numTokens=3'.
	    at <Jasmine>
	    at UserContext.<anonymous> (tfjs-layers/src/layers/preprocessing/category_encoding_test.ts:139:6 <- tfjs/tfjs-layers/src/tfjs-layers_test_bundle.js:55301:59)
	    at <Jasmine>

This is strange; the two error messages look identical to me (unless I've just completely missed something).

Same for output mode tests:

Chrome 105.0.0.0 (Mac OS 10.13.6) Tests for preprocessing utils cpu {} Thows an error if input rank > 2 FAILED
	Expected function to throw an exception with message 'When outputMode is not 'int', maximum output rank is 2
	    Received outputMode 'multiHot' and input shape 2,2,1
	    which would result in output rank 3.', but it threw an exception with message 'When outputMode is not 'int', maximum output rank is 2
	    Received outputMode multiHot and input shape 2,2,1
	    which would result in output rank 3.'.
	    at <Jasmine>
	    at UserContext.<anonymous> (tfjs-layers/src/layers/preprocessing/preprocessing_utils_test.ts:60:6 <- tfjs/tfjs-layers/src/tfjs-layers_test_bundle.js:55403:72)
	    at <Jasmine>

The failures are reproducible locally with yarn bazel run //tfjs-layers:tfjs-layers_webgl2_test (run in the root of the repository). You can focus the test that's failing by replacing it with fit, or you can use this url (gotten by clicking DEBUG in Chrome karma, waiting for the tests to finish, and then clicking the broken one in the list of tests), although you might need to change the port.

Edit: add url

Full logs for layers tests

@mattsoulanille
Copy link
Member

Oh, I see the issue. JS does not let you wrap strings by just adding a newline in them. Well, it does, but it includes the newline and all the indentation spaces. You'll need to split those strings into multiple different strings that you add together. I'll add an example.

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.

Quick review showing how to fix the multiline string errors. I'll do a full review in a bit.

AdamLang96 and others added 7 commits October 3, 2022 17:23
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
* fixed indentation and linting error

* fixed return statement in preprocessing function

* fixed multiline error message in preprocessing_utils.ts
* passing all unit tests
* revision for catencoding pr

* fixed tests and reviewer reccomendations

* fixed changed files for clean pr

* fixed indentation and linting error

* fixed return statement in preprocessing function

* commit change before upstream

* fixed multiline error message in preprocessing_utils.ts

* passing all unit tests

* fixed int string
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.

LGTM with a few nits. Thanks!

AdamLang96 and others added 3 commits October 4, 2022 10:26
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
Co-authored-by: Matthew Soulanille <matthew@soulanille.net>
* added reviewer requests
@AdamLang96
Copy link
Contributor Author

@mattsoulanille I just updated the branch and it caused the build to fail. Please let me know if there is something I can fix. Thanks for all of the help!

@mattsoulanille
Copy link
Member

@AdamLang96 We just changed some GCP Cloudbuild permissions and they accidentally broke CI for a few minutes. Should be fixed now, so I'll re-run the tests.

@mattsoulanille mattsoulanille merged commit 521f9d7 into tensorflow:master Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants