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

Update sphere.py -- add pooling and max_pooling #1596

Merged
merged 17 commits into from
Feb 20, 2024

Conversation

mzameshina
Copy link
Contributor

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context / Related issue

How Has This Been Tested (if it applies)

Checklist

  • The documentation is up-to-date with the changes I made.
  • I have read the CONTRIBUTING document and completed the CLA (see CLA).
  • All tests passed, and additional code has been covered with new tests.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 16, 2024
Copy link
Contributor

@teytaud teytaud left a comment

Choose a reason for hiding this comment

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

Please use the same signature as other codes so that it can work in the quasi-randomize function.

Please add your code as default in quasi-randomize, when len(shape) > 1, instead of dispersion_with_big_conv.

@@ -826,6 +855,8 @@ def metric_pack_big_conv(x, budget=default_budget):
"Riesz_blursum_lowconv_loworder",
"Riesz_blursum_lowconv_midorder",
"Riesz_blursum_lowconv_highorder",
"max_pooling",
"pooling"
Copy link
Contributor

Choose a reason for hiding this comment

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

All methods in the list have the same signature, so that they can be used by quasi-randomize.
Please use the same signature.

fix signature of pooling and max_pooling
fix style
@teytaud
Copy link
Contributor

teytaud commented Feb 18, 2024

Does it run ? This does not work as Torch is not in the dependencies.
I recommend not using torch, as it would imply a big dependency.

Also you need black.

@teytaud teytaud merged commit 75abae1 into facebookresearch:main Feb 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants