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

Created experimental folder and added all NTK sketching codes #142

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

insuhan
Copy link

@insuhan insuhan commented Feb 24, 2022

Created experimental folder and added all NTK sketching codes.

@google-cla
Copy link

google-cla bot commented Feb 24, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@jaehlee jaehlee self-requested a review March 9, 2022 19:30
@jaehlee jaehlee self-assigned this Mar 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #142 (a16098c) into main (a38c637) will increase coverage by 1.76%.
The diff coverage is 96.26%.

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   86.08%   87.84%   +1.76%     
==========================================
  Files          19       24       +5     
  Lines        4851     5868    +1017     
==========================================
+ Hits         4176     5155     +979     
- Misses        675      713      +38     
Impacted Files Coverage Δ
experimental/__init__.py 100.00% <ø> (ø)
experimental/tests/sketching_test.py 93.54% <93.54%> (ø)
experimental/features.py 93.69% <93.69%> (ø)
experimental/tests/features_test.py 97.96% <97.96%> (ø)
experimental/poly_fitting.py 100.00% <100.00%> (ø)
experimental/sketching.py 100.00% <100.00%> (ø)
neural_tangents/experimental/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a38c637...a16098c. Read the comment docs.

Copy link
Collaborator

@jaehlee jaehlee left a comment

Choose a reason for hiding this comment

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

Thank you for implementing these features, so far on small tests, the functionality looks good! Here are some small comments to start off the reviews.

Looking forward to see polysketch working, since so far 'rf' conv features don't approximate exact kernel as closely as desired.

experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/README.md Outdated Show resolved Hide resolved
experimental/README.md Show resolved Hide resolved
@jaehlee
Copy link
Collaborator

jaehlee commented Mar 17, 2022

Hello Amir! Thanks for committing the polysketch routines! Could you sign the contributor license agreement (CLA) as described below in failed checks? Google's github repo requires all the contributors to sign the CLA.

experimental/ntk_sketch.py Outdated Show resolved Hide resolved
experimental/ntk_sketch.py Outdated Show resolved Hide resolved
experimental/ntk_sketch.py Outdated Show resolved Hide resolved
experimental/ntk_sketch.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
Copy link
Contributor

@romanngg romanngg left a comment

Choose a reason for hiding this comment

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

Awesome progress! Left some initial comments, feel free to ignore/prioritize them as you see fit. May comment more later.

experimental/features.py Outdated Show resolved Hide resolved
experimental/test_fc_ntk.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
kappa0_mat = kappa0(nngp_feat_2d)
ntk_feat = cholesky(ntk * kappa0_mat).reshape(input_shape + (-1,))

else:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest having the same clause in init_fn above

experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/README.md Outdated Show resolved Hide resolved
@romanngg
Copy link
Contributor

Also, I suggest regularly syncing the fork (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork) to make sure the PR stays in sync with latest changes.

Copy link
Contributor

@romanngg romanngg left a comment

Choose a reason for hiding this comment

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

Left some more minor comments that would help us more easily sync this PR with internal codebase. Where possible, please try to to have https://github.com/google/neural-tangents/actions/workflows/pytype.yml pass, since this helps our sync process as well (but feel free to add pytype: disable=<error-code>, or ignore it altogether if the error messages don't make sense!).

experimental/ntk_sketch.py Outdated Show resolved Hide resolved
experimental/test_fc_ntk.py Outdated Show resolved Hide resolved
experimental/sketching.py Outdated Show resolved Hide resolved
experimental/sketching.py Outdated Show resolved Hide resolved
romanngg added a commit that referenced this pull request Mar 30, 2022
…u can run `build_cleaner third_party/py/neural_tangents/...` in the new copybara workspace to create BUILD targets. There's still a lot of stuff to improve (some changes suggested in #142, and here would be nice to run build_cleaner automatically), but this could be a bit helpful in the meantime.

PiperOrigin-RevId: 436878167
experimental/sketching.py Outdated Show resolved Hide resolved
experimental/sketching.py Outdated Show resolved Hide resolved
experimental/sketching.py Outdated Show resolved Hide resolved
experimental/tests/kernel_approx_test.py Outdated Show resolved Hide resolved
experimental/tests/myrtle_network_test.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/test_fc_ntk.py Outdated Show resolved Hide resolved
@romanngg
Copy link
Contributor

romanngg commented Apr 6, 2022

Regarding tests, it looks like Github allows for 20 concurrent jobs running https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits, and we currently have 8 (x32/x64 * 4 python versions) linux jobs + 1 pytype job + 2 sketching jobs (x32/x64), so I believe linux/pytype jobs should not slow down the sketching jobs. AFAIK in the last run the sketching job failed first within 25 minutes https://github.com/google/neural-tangents/runs/5845991851?check_suite_focus=true. I'm not sure of the reason, but perhaps reducing the size as I mention in other comments could help?

experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
experimental/features.py Outdated Show resolved Hide resolved
EXACT = 'EXACT'


def layer(layer_fn):
Copy link
Contributor

Choose a reason for hiding this comment

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

To have a clear picture of the Features API for users, it would be nice to type-annotate its layers and init_fn/feature_fn, by analogy to

InternalLayer = Tuple[InitFn, ApplyFn, LayerKernelFn]

and e.g.

(In your case InternalFeatureLayer = Tuple[FeatureInitFn, FeatureFn], and for the functions you can define the typing protocols like

)

experimental/tests/features_test.py Outdated Show resolved Hide resolved
experimental/tests/features_test.py Outdated Show resolved Hide resolved
experimental/poly_fitting.py Outdated Show resolved Hide resolved
@romanngg
Copy link
Contributor

Also, the readthedocs fail https://readthedocs.org/projects/neural-tangents/builds/17123387/ should go away after syncing the repo with the main branch

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.

6 participants