Improving the layer Description. #734
Merged
Conversation
This file contains 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
I have added clear information regarding what the layer does. It was not specified earlier, because of it user gets in trouble about what it does and when to use it.
mattdangerw
requested changes
Feb 10, 2023
| @@ -22,7 +22,9 @@ | |||
|
|
|||
| @keras.utils.register_keras_serializable(package="keras_nlp") | |||
| class TokenAndPositionEmbedding(keras.layers.Layer): | |||
| """A layer which sums a token and position embedding. | |||
| """Token and position embeddings are ways of representing words and their order in a sentence | |||
There was a problem hiding this comment.
The first line of a docstring should always be a single sentence fragment <80 characters. Let's leave this unchanged
We could add a description to the next paragraph though! Maybe....
Token and position embeddings give a dense representation of the
input tokens, and their position in the sequence respectively. This
layer create a `keras.layers.Embedding` token embedding and a
`keras_nlp.layers.PositionEmbedding` position embedding and sums
their output when called. This layer assumes that the last dimension in
the input corresponds to the sequence dimension.
Updated the docstring as suggested by the reviewer.
mattdangerw
requested changes
Feb 15, 2023
| @@ -23,7 +23,11 @@ | |||
| @keras.utils.register_keras_serializable(package="keras_nlp") | |||
| class TokenAndPositionEmbedding(keras.layers.Layer): | |||
| """A layer which sums a token and position embedding. | |||
|
|
|||
| Token and position embeddings are ways of representing words and their order in a sentence | |||
There was a problem hiding this comment.
- Add an newline between the docstring summary (the first line), and this one
- Let's delete "so that a machine learning model like a neural network can understand them" that feels a little under technical.
- Please format this so line lengths are <80 characters.
|
|
||
| Token and position embeddings are ways of representing words and their order in a sentence | ||
| so that a machine learning model like a neural network can understand them. This | ||
| layer create a `keras.layers.Embedding` token embedding and a |
I made some changes as the reviewer suggested.
|
Thanks! |
mattdangerw
added a commit
to mattdangerw/keras-hub
that referenced
this pull request
Feb 16, 2023
This broke on keras-team#734
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I have added clear information regarding what the layer does. It was not specified earlier, because of it user gets in trouble about what it does and when to use it.