Luthaf / rascaline

Computing representations for atomistic machine learning
https://luthaf.fr/rascaline/
BSD 3-Clause "New" or "Revised" License
44 stars 13 forks source link

Added real and Fourier space spliner base classes #235

Closed PicoCentauri closed 11 months ago

PicoCentauri commented 12 months ago

As given in the title this PR adds two new base clases:

  1. RealSpaceRadialIntegralSplinerBase
  2. FourierSpaceRadialIntegralSplinerBase

With these base classes constructing (almost) any radial integral becomes quite easy. A developer/user only has to implement the method for her/his radial basis in a child class. The actual integral is performed numerically with quad. Also I added the option to (ortho)normalize the radial integral.

For RealSpaceRadialIntegralSplinerBase I added support for delta atomic densities and Gaussian atomic densities. The support for custom atomic densities is prepared, but the logic is missing. Since the integral is a more complex we should do this in an upcoming PR.

FourierSpaceRadialIntegralSplinerBase already works for any atomic density and radial basis.

TODO

I still have to document and test everything. Especially the equations for the radial integral should be added.

As a test I suggest that I add a GTO child class for LODE and SOAP and compare this to the analytical results which we already implemented in rust.

I would happy about some comments if the class structure makes sense and is understandable :-).


:books: Documentation preview :books:: https://rascaline--235.org.readthedocs.build/en/235/

github-actions[bot] commented 12 months ago

Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping wheels.zip and using pip to install the file matching your system

Luthaf commented 11 months ago

Regarding the class structure, should we have separate classes for atomic density and basis functions instead of putting everyone inside the spliner? Something like

class GaussianDensity(AtomicDensityBase):
    ...

class DeltaDensity(AtomicDensityBase):
    ...

class RadiallyVaryingDensity(AtomicDensityBase):
    ...

class GTOBasis(RadialBasis):
    ...

class MonomialBasis(RadialBasis):
    ...

class RealSpaceSpliner():
    def __init__(self, density: AtomicDensityBase, basis: RadialBasis):
        ...

class KSpaceSpliner():
    def __init__(self, density: AtomicDensityBase, basis: RadialBasis):
        ...

This way RealSpaceSpliner and KSpaceSpliner can be concrete classes, and the users only need to customize the part they care about (density and/or basis).


A partially unrelated thing I find weird is making the abstractmethod functions private (starting with an _). If they are part of the abstract interface, they should be public IMO.

Which is also solved by the API above, using composition instead of inheritance for the density & basis functions!

PicoCentauri commented 11 months ago

Yes, we can split this up even more. Shouldn't we then use inheritance, i.e.

class GTORealSpaceSpliner(RealSpaceSpliner, GTOBasis, GaussianDensity):
    ....
Luthaf commented 11 months ago

I think composition is going to be much clearer than multiple inheritance here

PicoCentauri commented 11 months ago

Yeah might be. First of all if to fix the code that it is actually working. Currently, I run into infinite loops when doing the integrals.

PicoCentauri commented 11 months ago

@Luthaf sorry for the delay but I continued the work on the spliner classes. Splitting the AtomicDensities and the RadialBasis was a very good idea. Looks much cleaner now. There is still a lot of docs missing, but maybe you can another look?

The LODE part is already working and agrees with the analytical implementation that we did in rust as the test_fourier_space_spliner() is showing. 🥳

I will continue the work on the real space part next week.

PicoCentauri commented 11 months ago

After some rounds of debugging the implementation is working. The main and most important tests are testing that the Numerical implementation for the special case of GTOs + Gaussian densities agrees with the analytical implementation in rust for GTOs + Gaussian densities.

In addition I already added some more interesting densities and basis classes.

The theory part was written by @kvhuguenin 🙏. I just ported this from a LaTeX document to rst.

PicoCentauri commented 11 months ago

Thanks again for the feedback @Luthaf. If you want to give the docs another round of improvements I am happy if you provide a commit. Otherwise we can also proceed in another PR. This is already quite big...

Luthaf commented 11 months ago

CI error is pulling from host quay.io failed with status code [manifests latest]: 500 Internal Server Error, I'll try to re-run later.