This repository was archived by the owner on Jul 10, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 580
Update Keras categorical input design, mark it as complete. #267
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
More importantly, add an official guide on how TF1 Estimator users with feature columns can convert their code into TF2 Keras users with KPL.
goldiegadde
reviewed
Jul 15, 2020
| * Users with large dimension categorical inputs will incur large memory footprint and computation cost, if wrapped with indicator column through `tf.keras.layers.DenseFeatures`. | ||
| * Currently there is no way to correctly feed Keras linear model or dense layer with multivalent categorical inputs or weighted categorical inputs. | ||
| * Currently there is no way to correctly feed Keras linear model or dense layer with multivalent categorical inputs or weighted categorical inputs, or shared embedding inputs. | ||
| * feature columns offer black-box implementations, mix feature engineering with trainable objects, and lead to |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: feature/Feature
Contributor
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
goldiegadde
reviewed
Jul 15, 2020
ematejska
suggested changes
Jul 15, 2020
| # Keras categorical inputs | ||
|
|
||
| | Status | Accepted | | ||
| | Status | Completed | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean it's implemented? I would prefer the terminology Implemented instead of Completed. Could you also post the PR number where it was implemented? Something like:
Status: Implemented (Link to PR)
Contributor
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ematejska
approved these changes
Jul 15, 2020
fchollet
approved these changes
Jul 15, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only one minor change to split "IndexLookup" to "StringLookup" and "IntegerLookup". The rest is trivial.
But more importantly, add an official guide on how TF1 Estimator users with feature columns can convert their code into TF2 Keras users with KPL.