-
Notifications
You must be signed in to change notification settings - Fork 487
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
add an interface to eval descriptors #1483
Conversation
Codecov Report
@@ Coverage Diff @@
## devel #1483 +/- ##
==========================================
+ Coverage 75.99% 76.03% +0.04%
==========================================
Files 93 93
Lines 7674 7729 +55
==========================================
+ Hits 5832 5877 +45
- Misses 1842 1852 +10
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.
I would recommend provide an separate method for inferring the descriptors, because strictly speaking, they are intermediate variable, not the output of a model. The eval
method is provided to infer the output of a model.
if self.auto_batch_size is not None: | ||
return self.auto_batch_size.execute_all(self._eval_inner, numb_test, natoms, | ||
coords, cells, atom_types, fparam = fparam, aparam = aparam, atomic = atomic, efield = efield) | ||
return self._eval_inner(coords, cells, atom_types, fparam = fparam, aparam = aparam, atomic = atomic, efield = efield) | ||
else : | ||
if self.auto_batch_size is not None: | ||
e, f, v = self.auto_batch_size.execute_all(self._eval_inner, numb_test, natoms, | ||
coords, cells, atom_types, fparam = fparam, aparam = aparam, atomic = atomic, efield = efield) | ||
else: | ||
e, f, v = self._eval_inner(coords, cells, atom_types, fparam = fparam, aparam = aparam, atomic = atomic, efield = efield) | ||
if self.modifier_type is not None: | ||
me, mf, mv = self.dm.eval(coords, cells, atom_types) | ||
e += me.reshape(e.shape) | ||
f += mf.reshape(f.shape) | ||
v += mv.reshape(v.shape) | ||
return e, f, v |
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.
Any reason why auto_batch_size
is removed?
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 removed. Instead, it's moved into a wrapper eval_func
to make the code more clear.
deepmd/infer/deep_pot.py
Outdated
e += me.reshape(e.shape) | ||
f += mf.reshape(f.shape) | ||
v += mv.reshape(v.shape) |
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.
output
is not updated with me, mf, mv before return.
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 don't think so. Here is a minimal example:
>>> import numpy as np
>>> output = (np.ones(3), np.ones(3), np.ones(3), np.ones(3), np.ones(3))
>>> e,f,v = output[:3]
>>> e += 1
>>> f += 2
>>> v += 3
>>> output
(array([2., 2., 2.]), array([3., 3., 3.]), array([4., 4., 4.]), array([1., 1., 1.]), array([1., 1., 1.]))
See https://stackoverflow.com/a/35910888/9567349. In Python, a+=b
is different from a=a+b
. a=a+b
creates a new object, but a+=b
modifies the original 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.
by e,f,v = output[:3]
e, f, v are copies of the list, not refs of the list element. You may do a test as
In [1]: a = [1,2,3]
In [2]: a[2]+=1
In [3]: a
Out[3]: [1, 2, 4]
In [4]: e,f,v=a[:3]
In [5]: e+=1
In [6]: f+=1
In [7]: v+=1
In [8]: a
Out[8]: [1, 2, 4]
In [9]: e, f, v
Out[9]: (2, 3, 5)
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 can reproduce your example.
It seems that python does not copy the numpy arrays , but do copy numbers.
Adding modification in this way confuses the readers of the code, I suggest you changing it to
output[0] += me.reshape(e.shape)
...
deepmd/infer/deep_pot.py
Outdated
e += me.reshape(e.shape) | ||
f += mf.reshape(f.shape) | ||
v += mv.reshape(v.shape) |
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 can reproduce your example.
It seems that python does not copy the numpy arrays , but do copy numbers.
Adding modification in this way confuses the readers of the code, I suggest you changing it to
output[0] += me.reshape(e.shape)
...
Fix #1393.