Solves #1512 - Githubissues

ml-explore / mlx

MLX: An array framework for Apple silicon

https://ml-explore.github.io/mlx/

MIT License

17.59k stars 1.02k forks source link

Solves #1512 #1526

Closed Saanidhyavats closed 1 week ago

Saanidhyavats commented 1 month ago

Proposed changes

Solves #1512

Added pool3d class and maxpool3d in pooling.py file

Please include a description of the problem or feature this PR is addressing. If there is a corresponding issue, include the issue #.

Checklist

Put an x in the boxes that apply.

[x] I have read the CONTRIBUTING document
[x] I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
[x] I have added tests that prove my fix is effective or that my feature works
[x] I have updated the necessary documentation (if needed)

Saanidhyavats commented 1 month ago

@awni We have implemented Maxpool3d and Avgpool3d, if it looks good then we can proceed with adding tests to it.

angeloskath commented 4 weeks ago

I think after merging the pooling was removed?

Could you add pool3d back, rebase on main and push force to the same branch? Then I will run the tests and merge if they pass.

Also, sorry for the nitpicking but do you mind putting _Pool3d together with _Pool2d class (higher in the file), implementing average pooling 3d and writing docstrings for the new layers? The docstrings don't have to be as verbose as for the rest of the pooling layers but they should exist at least.

Thanks!

Saanidhyavats commented 4 weeks ago

I think after merging the pooling was removed?

Could you add pool3d back, rebase on main and push force to the same branch? Then I will run the tests and merge if they pass.

Also, sorry for the nitpicking but do you mind putting _Pool3d together with _Pool2d class (higher in the file), implementing average pooling 3d and writing docstrings for the new layers? The docstrings don't have to be as verbose as for the rest of the pooling layers but they should exist at least.

Thanks!

@angeloskath We have made the suggested changes. I think this is ready to merge. Let me know if anything else is required from our side.

cvnad1 commented 3 weeks ago

@angeloskath do you think anything needs any correction?

Saanidhyavats commented 2 weeks ago

@angeloskath @awni Could you confirm if everything is fine, and if so, would it be possible to merge it?