Skip to content

Conversation

@jorisSchaller
Copy link
Contributor

Moved pooling function out of linen according to the comment #4408 (comment)

What does this PR do?

It moves the pooling function from linen to flax

I moved the pooling operation as they can be better shared between Linen and nnx.
The documentation was also missing from the nnx documentation. This PR adds an element in the toc as well as a page describing the function

Fixes #4271
closes #4408

Checklist

  • This PR fixes a minor issue (e.g.: typo or small bug) or improves the docs (you can dismiss the other checks if that's the case).

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 29, 2025

@jorisSchaller thanks for the PR, can you please rebase it on the latest main to avoid unrelated changes? Thanks!

@jorisSchaller
Copy link
Contributor Author

@vfdev-5 let me know if you'd like any additional changes

@jorisSchaller
Copy link
Contributor Author

@vfdev-5 reverted changes and merged upstream changes

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 31, 2025

@jorisSchaller please check this failing test: https://github.com/google/flax/actions/runs/18982441063/job/54231421369?pr=5057

@jorisSchaller
Copy link
Contributor Author

@vfdev-5 Updated

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 2, 2025

@jorisSchaller thanks for updates on the PR !
We still need to fix few things:

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 10, 2025

@jorisSchaller please rebase on main and fix the failing check with ruff: https://github.com/google/flax/actions/runs/19210434754/job/54950332158?pr=5057

@vfdev-5 vfdev-5 force-pushed the feat/nnx_pooling branch 2 times, most recently from a107279 to 3146d14 Compare December 5, 2025 08:41
- Pooling operation moved out of Linen
- Pooling documentation duplicated in nnx
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks @jorisSchaller for PR. I finished the remaining things and now it LGTM

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 documentation missing pooling operations

2 participants