-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
DFT Part-3 #3817
DFT Part-3 #3817
Conversation
[skip ci]
@@ -0,0 +1,542 @@ | |||
import torch |
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.
Don't have _jacobian.py, rename to jacobian.py
|
||
|
||
class Jacobian(object): | ||
"""Base class for the Jacobians used in rootfinder algorithms.""" |
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.
Add a longer comment here; this is an important class
>>> jacobian.solve(v) | ||
tensor([-0.7071, -0.7071], grad_fn=<MulBackward0>) | ||
|
||
[1].. B.A. van der Rotten, PhD thesis, |
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.
This should be in a references section
|
||
""" | ||
|
||
def __init__(self, alpha=None, uv0=None, max_rank=None): |
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.
Missing type annotations
# taking most of the part from SciPy | ||
|
||
|
||
class Jacobian(object): |
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.
Add type annotations on all this code
|
||
|
||
class LinearMixing(Jacobian): | ||
""" Approximating the Jacobian based on linear mixing. |
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.
Add details on the algorithm. I'm not familiar with LinearMixing
|
||
|
||
class LowRankMatrix(object): | ||
"""represents a matrix of `\alpha * I + \sum_n c_n d_n^T` |
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.
More details here
del self.dns[:n - max_rank] | ||
|
||
|
||
class FullRankMatrix(object): |
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.
More details here; type annotations
@@ -0,0 +1,161 @@ | |||
import warnings | |||
import torch |
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.
For code adapted from xitorch, let's add links in docstring to original source
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.
Not in bad shape but needs more documentation and type annotations primarily. Once those are fixed and CI is running again good to merge
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.
LGTM
This is a large PR but checking I think there are no new failures.
@GreatRSingh Let's coordinate at next OH about remaining dqc/xitorch port PRs
Description
Fix #(issue)
Type of change
Please check the option that is related to your PR.
Checklist
yapf -i <modified file>
and check no errors (yapf version must be 0.32.0)mypy -p deepchem
and check no errorsflake8 <modified file> --count
and check no errorspython -m doctest <modified file>
and check no errors