G
YouTube TikTok Games
Skip to content

Add the missing b_hn bias to nnx.GRUCell - #5645

Open
vineethsaivs wants to merge 1 commit into
google:mainfrom
vineethsaivs:fix-nnx-grucell-hn-bias
Open

vineethsaivs wants to merge 1 commit into
google:mainfrom
vineethsaivs:fix-nnx-grucell-hn-bias

Conversation

@vineethsaivs

Copy link
Copy Markdown

Closes #5594. Replaces #5595, which was closed when main was force-pushed to its branch.

nnx.GRUCell documents n = tanh(W_in x + b_in + r * (W_hn h + b_hn)), and linen.GRUCell builds b_hn as the bias of its hn layer. nnx fuses r, z and n into one bias-free dense_h, so b_hn was missing.

Fix: add it as its own hn_bias parameter (a bias on the fused dense_h would also reach r and z), promoted and broadcast like Linear's bias.

Test: no new tests, per review. With weights copied from linen.GRUCell and a non-zero bias_init, the outputs match; on main they do not. tests/nnx/nn/recurrent_test.py passes on CPU (20 tests).

Compatibility: one extra array in the parameter tree, so a checkpoint saved from the old nnx.GRUCell needs a fallback to restore. bias_init defaults to zeros, so a fresh cell is numerically unchanged.

The class docstring gives the cell as
n = tanh(W_in x + b_in + r * (W_hn h + b_hn)), and flax.linen.GRUCell
builds b_hn as the bias of its `hn` layer. nnx fuses r, z and n into one
`dense_h` with use_bias=False, so no hidden bias exists and __call__
computes n = activation_fn(xi_n + r * hh_n).

A bias on the fused layer would also reach r and z, so carry it as its
own parameter, which is what linen does with a separate `hn` bias.

bias_init defaults to zeros, so a freshly initialised cell is unchanged;
the divergence only appears once that bias is trained or when weights are
ported from linen or torch.nn.GRUCell.

The bias is promoted with promote_dtype and reshaped to hh_n's rank, as
Linear does, so batched carries do not rely on implicit rank promotion and
a bfloat16 carry stays bfloat16.
Comment thread flax/nnx/nn/recurrent.py
hh_n, hn_bias = self.promote_dtype(
(hh_n, self.hn_bias[...]), dtype=self.dtype
)
hh_n += jnp.reshape(hn_bias, (1,) * (hh_n.ndim - 1) + (-1,))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note: I think the reshape here is unnecessary. This is already what broadcast rules give you for free.

This branch has not been deployed

No deployments
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.

nnx.GRUCell is missing the b_hn bias that its docstring and linen.GRUCell both specify

2 participants