uf3 icon indicating copy to clipboard operation
uf3 copied to clipboard

improve compatibility of UFCalculator with ASE

Open sunghjung3 opened this issue 2 years ago • 2 comments

The UFCalculator was slightly modified to improve the compatibility with the ASE calculator. Only the layout of the UFCalculator class and its methods have been changed but not any of its core computational codes.

Practically speaking, there were 2 major issues with the current UFCalculator:

  1. No caching of energy/force values.

    • With an ASE Atoms object with several hundred atoms, it may take a couple of minutes for a forcecall. In a proper ASE calculator, the calculated energy and force values are cached in a dictionary called self.results so that if none of the attributes of the Atoms object (i.e. positions, cell vectors, etc) have changed since the last forcecall, these cached values can be fetched instead of doing another forcecall.
  2. Trouble with saving to ASE trajectory files

    • This issue is described in #46 .

When atoms.get_<property>() is called, it eventually is redirected to atoms.calc.get_property(<property>, atoms) (defined in ase/calculators/calculator.py), where it checks to see if any attributes of atoms have changed and if the <property> already exists in atoms.calc.results. If a new calculation is necessary, then the atoms.calc.calculate method is called, where the actual calculation is performed. This is the only new method I have added to UFCalculator. Then, this new calculate method calls the old UFCalculator.get_<property> methods, which I have renamed to _get_<property> to not overlap with any inherited methods and not to confuse ourselves with the get_<property> methods of the Atoms object.

Now, Atoms objects with UFCalculator can be attached and written to ASE trajectory files, and energy and force calculations work as intended. For stresses, since it uses ASE's numerical force calculator in the backend, the caching feature cannot be used for stresses - the numerical force calculator uses finite difference, and every time it rattles the Atoms object, its cache gets wiped out (they should have kept a copy of the original object and do the calculation with a copy! but whatever). But if we ever implement analytical stresses, the caching should work.

For elastic constant and phonon calculations, I decided to leave it as it is for now because get_elastic_constants and get_phonon_data are not defined methods of the Atoms class, and its caching situation seemed similar to that of numerical stress.

sunghjung3 avatar Feb 08 '24 23:02 sunghjung3

The code looks good to me. I would add a bit more documentation in the code and maybe a new unitest. There's only one unittest for UFCalculator for unary system, maybe we can add one more unittest for a binary system like CH4.

The unitest can also be part of a different pull request.

From my side to approve this pull request only a bit more code documentation is needed.

monk-04 avatar Feb 09 '24 15:02 monk-04

I've added some documentation for the calculate method.

I'll leave the unit test to a future pull request. In my opinion, I think a test for 3-body is more necessary than a binary system, but it is not clear to me yet how I would write an analytical solution for it.

sunghjung3 avatar Feb 09 '24 17:02 sunghjung3

I've added some more unit tests and have verified that they pass both before and after these changes.

sunghjung3 avatar May 31 '24 19:05 sunghjung3

Is the pull request ready? Or are you still working on more changes?

monk-04 avatar May 31 '24 20:05 monk-04

@monk-04 It's ready

sunghjung3 avatar May 31 '24 20:05 sunghjung3

Ok, done for real now

sunghjung3 avatar May 31 '24 22:05 sunghjung3

Approving!!

monk-04 avatar May 31 '24 22:05 monk-04