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

Solves #1512 #1526

Merged
merged 13 commits into from
Nov 20, 2024
Merged

Solves #1512 #1526

merged 13 commits into from
Nov 20, 2024

Conversation

Saanidhyavats
Copy link
Contributor

@Saanidhyavats Saanidhyavats commented Oct 25, 2024

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.

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

@Saanidhyavats
Copy link
Contributor Author

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

@angeloskath
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

cvnad1 commented Nov 7, 2024

@angeloskath do you think anything needs any correction?

@Saanidhyavats
Copy link
Contributor Author

Saanidhyavats commented Nov 10, 2024

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

Copy link
Member

@angeloskath angeloskath left a comment

Choose a reason for hiding this comment

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

Looks good!

@angeloskath angeloskath merged commit cb431df into ml-explore:main Nov 20, 2024
1 of 2 checks passed
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