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

Add RMSNorm #629

Merged
merged 4 commits into from
Dec 28, 2023
Merged

Add RMSNorm #629

merged 4 commits into from
Dec 28, 2023

Conversation

jondeaton
Copy link
Contributor

Branched from #545 addressing reviewers' comments.

Copy link
Owner

@patrick-kidger patrick-kidger left a comment

Choose a reason for hiding this comment

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

Doc nit aside, this LGTM! Thank you for tidying this up.
I'll merge this into the new dev branch shortly.

[this paper](https://browse.arxiv.org/abs/2307.14995). `\beta` is an optional bias
term.

??? cite
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: this will need a new line here in order to render correctly in the docs.

i.e.

??? cite

    foo

Copy link
Owner

Choose a reason for hiding this comment

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

Also, whilst we're here -- maybe worth emphasising that use_{weight,bias}=False is the default.

(Should we keep it as the default? I appreciate that's maybe more useful, but it's inconsistent with the other normalisation layers. WDYT?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for raising the point about the defaults. I feel its good to maintain consistency, especially with LayerNorm. Changed defaults to on.

@patrick-kidger patrick-kidger changed the base branch from main to dev December 27, 2023 19:30
@patrick-kidger patrick-kidger merged commit b51faf9 into patrick-kidger:dev Dec 28, 2023
2 checks passed
@patrick-kidger
Copy link
Owner

Alright, LGTM! Thank you for tidying this one up.
I've merged it into the dev branch, so this will appear in the next release of Equinox :)

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.

3 participants