-
Notifications
You must be signed in to change notification settings - Fork 8
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
Feature/344 functorch #378
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstrings should use the same rst style.
y = hessian_vp(x) + hessian_perturbation * x | ||
return cp.from_dlpack(to_dlpack(y)) | ||
|
||
eigen_vals, eigen_vecs = eigsh( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of repetition and return points in this function. I think it should be simplified by making a single call to eigsh
and defining the different variables and functions in the if and else branches, along with an extra function to convert the result to torch tensors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, done
@@ -21,7 +21,7 @@ | |||
], | |||
setup_requires=["wheel"], | |||
tests_require=["pytest"], | |||
extras_require={"influence": ["torch"]}, | |||
extras_require={"influence": ["torch>=2.0.0"], "cupy": ["cupy-cuda11x>=12.1.0"]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cupy is only used with influence functions, right? What would be the disadvantage of adding it to the influence extras instead of creating a separate one just for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On mac, there is no wheel cupy-cuda11x. If we include it into the influence extra, then no mac user could install the influence extra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, we can leave it for now.
I will create a separate issue to consider using environment markers for this.
@schroedk could you please merge develop into your branch and mark this PR as ready so that CI can run? |
* remove parameter x0 * simplify if/else condition
@AnesBenmerzoug as this PR is not into develop, I would first merge into into to the target feature branch and run the CI there |
Changes
Checklist
- [ ] If notebooks were added/changed, added boilerplate cells are tagged with"nbsphinx":"hidden"