-
Notifications
You must be signed in to change notification settings - Fork 68
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
Stress kernels and other kernel related changes #187
Stress kernels and other kernel related changes #187
Conversation
…ir-group/flare into jon/feature/kernel-edits-merge
if name not in self.results.keys(): | ||
if not allow_calculation: | ||
return None | ||
self.calculate(atoms) | ||
self.calculate(atoms, structure) |
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 think adding structure
to the args here is because the predict_on_structure
function assigns energy, forces etc. to the structure
.
But I feel we need to re-consider the interface here. Since for other ASE calculator, people just do something like atoms.get_force()
etc., if structure
is needed to input to the function, the interface will not be consistent with other calculators.
To address this, I think of 2 ways, but there might be better approaches.
- we can have a separate function like (just give a rough idea)
def set_struc_attr(structure, forces, energies, ......):
structure.forces = forces
....
Then we just call set_struc_attr
after predict_on_structure
if the structure needs modifying
- we can add a new function like
predict_and_write_to_structure
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 interface as implemented is consistent with the general ASE calculator template described here. Note that our get_forces, get_potential_energy, and get_stress methods take in an atoms object only; the structure object is introduced as an optional input to the auxiliary get_property method.
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.
Actually all the get_forces
, get_stress
etc. calls get_property
, the reason atoms.get_forces()
works in otf
is that the line 144 (ase/otf.py)
self.atoms.calc.calculate(self.atoms, self.structure)
called the calculate
function, so there is a dict self.atoms.calc.results
which has been written the FLARE calculated forces
, energy
etc. Then when the atoms.get_forces
is called, it will just get the value from the results
dict without re-calculating of GP, so it's ok at this time if structure=None
.
This means that it only works in the otf
context. If we want to create a FLARE_Calculator without OTF, and try to use get_forces
, the structure has to be input, because at this time results
dict is empty, and calculate
function needs calling.
But I notice that structure
and ASE Atoms are very similar, so probably a better way to do this is just in ASE_OTF
let
self.structure = self.atoms
since they have a lot of common attributes like positions, cell etc. so there should only be some minor changes. may have to think about this. I will try to deal with this in another PR
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.
Hmm I actually think get_forces, get_stress etc. should still work without giving a structure. If a structure isn't given, the calculate method creates a structure based on the atoms object (see lines 69-73 of ase/calculator, and note that calculate is called by get_property, which sets structure to None if none is given). But I agree this isn't a perfect solution, and it would be better to completely eliminate structure from the ase calculator. This would require adding some extra calculator properties (energy, force, stress uncertainties) and then writing them to the structure somewhere else, probably in a method of ase/otf.
This is a step toward NPT on-the-fly training with the GP. Summary of major changes/additions:
mc_simple
test_stress_kernels
two_body_mc_simple, three_body_mc_simple
predict
gp_algebra
ase/calculator
ase/otf
output
struc