PR on nnlda layer #3237
PR on nnlda layer #3237
Conversation
deepchem/models/dft/nnxc.py
Outdated
| Neural network for xc functional | ||
| ninpmode: int | ||
| The mode to decide the transformation of the density to NN input. | ||
| outmultmode: int |
There was a problem hiding this comment.
Can you add a usage example and a returns section to the docstring?
deepchem/models/dft/nnxc.py
Outdated
| self, | ||
| xcstr: str, | ||
| nnmodel: torch.nn.Module, | ||
| *, |
There was a problem hiding this comment.
- arguments are a bad pattern imo. We shouldn't use them in the deepchem codebase.
deepchem/models/dft/nnxc.py
Outdated
| super().__init__() | ||
| # What is type of xc here? | ||
| self.xc = get_xc(xcstr) | ||
| print(type(self.xc)) |
There was a problem hiding this comment.
Print statements should be removed before merge
deepchem/models/dft/nnxc.py
Outdated
|
|
||
| class NNLDA(BaseNNXC): | ||
| """ | ||
| Neural network xc functional of LDA (only receives the density as input) |
There was a problem hiding this comment.
Can you add a usage example? There should be an explanation of what LDA is as a DFT method, along with a suitable reference. Aim for at least a few paragraphs of discussion
deepchem/models/dft/nnxc.py
Outdated
| nnmodel: torch.nn.Module | ||
| Neural network for xc functional | ||
| ninpmode: int | ||
| The mode to decide the transformation of the density to NN input. |
There was a problem hiding this comment.
What are the valid values? What does each value mean?
There was a problem hiding this comment.
Currently we only support one mode, so I have removed it. Will add it back in the next iterations.
deepchem/models/dft/nnxc.py
Outdated
| Neural network for xc functional | ||
| ninpmode: int | ||
| The mode to decide the transformation of the density to NN input. | ||
| outmultmode: int |
There was a problem hiding this comment.
What are the valid values for the output? What does each value mean?
There was a problem hiding this comment.
Currently we only support one mode, so I have removed it. Will add it back in the next iterations.
deepchem/models/dft/nnxc.py
Outdated
| try: | ||
| from dqc.xc.base_xc import BaseXC | ||
| from dqc.utils.datastruct import ValGrad, SpinParam | ||
| from dqc.api.getxc import get_xc |
There was a problem hiding this comment.
This looks like a utility function. Can you copy this in directly from dqc?
There was a problem hiding this comment.
Are we trying to do that in this iteration? If so, I'm not so sure, but I will look into it.
deepchem/models/dft/nnxc.py
Outdated
|
|
||
| class BaseNNXC(torch.nn.Module): | ||
| """ | ||
| Base class for the NNLDA and HybridXC classes. |
There was a problem hiding this comment.
Can you add more detail on when we would want to use this base class and what capabilities it provides? A good paragraph of explanation would be my rule of thumb
|
|
||
| Returns | ||
| ------- | ||
| res |
deepchem/models/dft/nnxc.py
Outdated
| class HybridXC(BaseNNXC): | ||
| """ | ||
| The HybridXC module computes XC energy by summing XC energy computed | ||
| from libxc and the trainable neural network with tunable weights. |
There was a problem hiding this comment.
Can you add a brief explanation of what libxc is?
The CI is fine. But I don't have merge access, it is blocked. Thanks |
Description
PR on nnlda layer