-
Notifications
You must be signed in to change notification settings - Fork 528
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
nvnmd #1707
nvnmd #1707
Conversation
Codecov Report
@@ Coverage Diff @@
## devel #1707 +/- ##
==========================================
+ Coverage 76.08% 77.10% +1.01%
==========================================
Files 96 118 +22
Lines 7927 9411 +1484
==========================================
+ Hits 6031 7256 +1225
- Misses 1896 2155 +259
Continue to review full report at Codecov.
|
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.
Before further review, I have the following general comments:
(1) Translate all Chinese comments into English;
(2) Format codes under deepmd/nvnmd
with pep8
;
(3) Add doc to each new class and methods to explain the role of them;
(4) Reduce data in the examples
diretory. As an example, one data set is enough.
(5) Add unit tests for new codes;
(6) Use os.path.join
to combine paths instead of /
.
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.
I notice that all examples are removed. What is the reason?
Please follow @njzjz 's comment :
(4) Reduce data in the examples diretory. As an example, one data set is enough.
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.
All C++ code implementing an OP (can be tested without TF C++ interface), should be moved from source/op to source/lib/src.
UTs of the OP implementation should be added.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
.Input("dv: T") | ||
.Input("grad_v: T") | ||
.Input("grad_dv: T") | ||
.Attr("prec: float") |
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.
Here prec
is float but x
may be either double or float. I think this cannot improve accuracy to double when you multiply x
by prec
.
deepmd/nvnmd/utils/config.py
Outdated
self.quantize_descriptor = True | ||
self.quantize_fitting_net = True | ||
else: | ||
pass |
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.
raise RuntimeError
if it's not expected behavior
This comment was marked as resolved.
This comment was marked as resolved.
log = logging.getLogger(__name__) | ||
|
||
|
||
class Encode(): |
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.
A lot of codes don't have the unit test, for example this class. See details of converge:
https://app.codecov.io/gh/deepmodeling/deepmd-kit/compare/1707/tree/deepmd/nvnmd
Code quality is acceptable. ping @wanghan-iapcm for more comments |
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 size of the data files coord.npy
and force.npy
are 1M. Could you please reduce the number of frames to make the files as small as possible? I would say 0.1M would be ideal. They present only for the purpose of showing the code is working.
deepmd/nvnmd/descriptor/se_a.py
Outdated
|
||
|
||
def descrpt2r4(inputs, natoms): | ||
""" replace :math:`r_{ji} \rightarrow r'_{ji}` |
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.
""" replace :math:`r_{ji} \rightarrow r'_{ji}` | |
r""" replace :math:`r_{ji} \rightarrow r'_{ji}` |
otherwise \r
will be recognized as the line separator.
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
Co-authored-by: LiuGroupHNU <liujie123@HNU> Co-authored-by: MoPinghui <mopinghui1020@gmail.com> Co-authored-by: Han Wang <92130845+wanghan-iapcm@users.noreply.github.com>
Our training procedure consists of not only the continuous neural network (CNN) training, but also the quantized neural network (QNN) training which uses the results of CNN as inputs, to help users train machine-learning models that are compatible with the unique non von Neumann computer.