Skip to content

Replace kernel computation in kgof with hyppo's compute_kern #322

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.

Sign up for GitHub

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

Jump to bottom
Open
darsh-patel opened this issue May 10, 2022 · 3 comments · May be fixed by #432
Open

Replace kernel computation in kgof with hyppo's compute_kern #322

darsh-patel opened this issue May 10, 2022 · 3 comments · May be fixed by #432
Assignees
@tjoab
Labels
enhancement New feature or request

Comments

@darsh-patel
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Currently, the kernel goodness-of-fit library kgof has its own unique implementation of a kernel evaluation due to a unique data structure returned by autograd.numpy gradient calculations. This ArrayBox data structure does not share properties with numpy arrays and therefore must be handled separately and cannot be fed into compute_kern.

Describe the solution you'd like
Ideally, a new implementation would rip and replace kernel.py by removing its kernel evaluation functions and reducing the corresponding dependencies on autograd.numpy to do so. Should be a quick fix if handling of an ArrayBox can be improved or a better gradient calculation implementation is discovered.

Describe alternatives you've considered
There has already been an attempt to integrate compute_kern by replacing the eval functions in kernel.py with instances of compute_kern, but this raises an error in which values in an ArrayBox cannot be reached.

Additional context (e.g. screenshots)
@darsh-patel darsh-patel added the enhancement New feature or request label May 10, 2022
@tjoab
Copy link

tjoab commented Apr 5, 2025 •
edited
Loading

Hey! Newbie contributor here with some questions about putting this feature request to rest. Excited to start contributing to some open source 😄 .

  1. From my deep dive, the crux of the problem here is the ArrayBox object. It seems that this is just what autograd.numpy uses within it gradient tracking and there is no way around it. If you unpack its value, to then feed into compute_kern(), you actually break the computational tree which is why autograd.numpy yells at us 😢. This is where my next question comes.

  2. Can we assume that kernel.py will only implement kernels with nice analytic gradients? If so, then I think the best call would be to just scrap autograd.numpy completely and just compute the gradients. This keeps everything as an 'ndarray' and should be compatible with compute_kern() and sklearn functions. The only issue is if we expect to implement kernels that do not have nice analytical derivatives, this is not the best idea.

Would love to be assigned on this and see what you guys think about the analytical approach. If that is something that would be desired, I can start working on a draft PR.

@darsh-patel
Copy link
Contributor Author

darsh-patel commented Apr 5, 2025 via email

@tjoab tjoab linked a pull request Apr 6, 2025 that will close this issue
Open
@sampan501
Copy link
Collaborator

@tjoab Thankk you for your comment! I like 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tjoab tjoab

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
3 participants
@sampan501 @tjoab @darsh-patel