Skip to content
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

Added real and Fourier space spliner base classes #235

Merged
merged 2 commits into from
Oct 17, 2023
Merged

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Sep 22, 2023

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 :-).


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

@github-actions
Copy link

github-actions bot commented Sep 22, 2023

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
Copy link
Member

Luthaf commented Sep 25, 2023

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
Copy link
Contributor Author

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

class GTORealSpaceSpliner(RealSpaceSpliner, GTOBasis, GaussianDensity):
    ....

@Luthaf
Copy link
Member

Luthaf commented Sep 26, 2023

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

@PicoCentauri
Copy link
Contributor Author

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 PicoCentauri force-pushed the r+k_spliner branch 2 times, most recently from 9a5c938 to 570fa99 Compare October 6, 2023 15:35
@PicoCentauri
Copy link
Contributor Author

@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 PicoCentauri force-pushed the r+k_spliner branch 6 times, most recently from 83021e1 to f6955a8 Compare October 10, 2023 09:40
@PicoCentauri PicoCentauri marked this pull request as ready for review October 10, 2023 09:40
@PicoCentauri
Copy link
Contributor Author

PicoCentauri commented Oct 10, 2023

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.

Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. I'll try to reflow the docs a bit, but that's easier to do as a commit instead of github suggestions.

python/rascaline/rascaline/utils/splines.py Outdated Show resolved Hide resolved
python/rascaline/rascaline/utils/atomic_density.py Outdated Show resolved Hide resolved
docs/src/explanations/radial-integral.rst Outdated Show resolved Hide resolved
docs/src/explanations/radial-integral.rst Outdated Show resolved Hide resolved
.. _k-space-radial-integral-1:

K-space Radial Integral
-----------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have to write it 🤣 pinging @kvhuguenin here because heis the master for this. We can remove the section for now.

python/rascaline/rascaline/utils/radial_basis.py Outdated Show resolved Hide resolved
assert_allclose(coeffs_num, coeffs_exact)


# def test_rspace_radial_integral():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Becuase, I still need to fix the analytical expression. 🫣

max_angular=max_angular,
k_cutoff=k_cutoff,
basis=basis,
density=DeltaDensity(), # density does not enter in a Kspace radial integral
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the atomic density does not enter the K-space RI, should we remove the parameter from the constructor?

Copy link
Contributor Author

@PicoCentauri PicoCentauri Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it does for the center contribution which we are not testing here.

python/rascaline/tests/utils/splines.py Show resolved Hide resolved
@@ -401,3 +479,380 @@ def _radial_integral_derivative(
return super()._radial_integral_derivative(n, ell, positions)
else:
return self.radial_integral_derivative_funcion(n, ell, positions)


class RealSpaceSpliner(RadialIntegralSplinerBase):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be called SoapSpliner and LodeSpliner instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can to be consistent with our namespace.

@PicoCentauri PicoCentauri force-pushed the r+k_spliner branch 4 times, most recently from 5bf424a to 4c999a7 Compare October 12, 2023 10:30
@PicoCentauri
Copy link
Contributor Author

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...

Co-authored-by: Guillaume Fraux <[email protected]>
@Luthaf
Copy link
Member

Luthaf commented Oct 13, 2023

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.

Copy link
Contributor Author

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the really nice improvements @Luthaf !!!

I am happy with the PR and hope that this can be helpful and gives us more flexibility of the basis and density combinations.

@Luthaf Luthaf merged commit 581d0ca into master Oct 17, 2023
24 checks passed
@Luthaf Luthaf deleted the r+k_spliner branch October 17, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants