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

flax.linen.Embed documentation could be improved #3458

Closed
hawkinsp opened this issue Nov 3, 2023 · 4 comments · Fixed by #3539
Closed

flax.linen.Embed documentation could be improved #3458

hawkinsp opened this issue Nov 3, 2023 · 4 comments · Fixed by #3539
Assignees

Comments

@hawkinsp
Copy link
Member

hawkinsp commented Nov 3, 2023

https://flax.readthedocs.io/en/latest/api_reference/flax.linen/_autosummary/flax.linen.Embed.html

  • An example of using the module would be incredibly helpful. In fact, it would be incredibly helpful if every flax.linen module had an example in its documentation.
  • The docs say:
A parameterized function from integers [0, n) to d-dimensional vectors.

num_embeddings
number of embeddings.

It wasn't immediately obvious to me that "number of embeddings" really meant "n", which is often called the vocabulary size.

  • In
features
number of feature dimensions for each embedding.

it is also slightly unclear that this is the same thing as d. Either use the same symbol, or refer back to the name used above?

  • I'll also note that the attend and setup methods are either undocumented or the links to their documentation aren't working.
@chiamp chiamp self-assigned this Dec 7, 2023
@chiamp
Copy link
Collaborator

chiamp commented Dec 7, 2023

Sorry for the delay, #3453 added example usages in the docstrings of each Module layer.
#3539 will additionally add more detail to the Embed documentation.

@jmerskine1
Copy link

The link for the updated documentation seems broken. I could be wrong, but there doesn't seem to be any example either. Has this correction been redacted?

@chiamp
Copy link
Collaborator

chiamp commented Apr 25, 2024

@jmerskine1
Copy link

@chiamp Thanks very much for your super quick response!

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 a pull request may close this issue.

3 participants