General spliner class #212

PicoCentauri · 2023-08-01T13:23:18Z

Replace generate_splines by a more flexible class
Allow to calculate the derivative of the radial integral numerically
Add linter for to check docstrings for double backticked strings which should be links to the Python documentation
Some minor cleanup to the rascaline __init__.py, i.e. adding an __all__ list.

Splitting the code into a SplinerBase and SplinerFromFunction class sounds maybe like an overkill here. However, this is only the first commit and should serve as a discussion basis for the architecture of the classes. After we agree on the design I will use the base class to extend the splining classes to more real and also kspace splining classes.

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

github-actions · 2023-08-01T16:25:26Z

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

frostedoyster

The design looks good to me, although the derived class isn't doing much additional work. Maybe we could ask for a third person's opinion on this

python/rascaline/tests/utils/splines.py

python/rascaline/rascaline/utils/splines.py

PicoCentauri · 2023-08-31T11:41:27Z

We have to repbase again metatensor but otherwise should be fine.

Luthaf

looks good, I have some comments on minor API points & naming

python/rascaline-torch/rascaline/torch/__init__.py

python/rascaline/rascaline/__init__.py

python/rascaline/rascaline/utils/__init__.py

Luthaf · 2023-09-20T12:02:09Z

python/rascaline/rascaline/utils/splines.py

+        Note that the integration range was deliberately left ambiguous since it depends
+        on the radial basis, i.e. for the GTO basis, :math:`r \in R^+` is used, while
+        :math:`r \in [0, cutoff]` for the monomial basis.


I still disagree with this formulation ^_^. this is always an integral to +\infty, but sometime the function to integrate is 0 outside of the cutoff, so the integral can be simplified. But we can always define it as an integral from 0 to +\infty.

fine for me to change it.

python/rascaline/rascaline/utils/splines.py

Luthaf · 2023-09-20T12:09:01Z

python/rascaline/rascaline/utils/splines.py

+    :parameter center_contribution: Contribution of the central atom required for LODE
+        calculations.


This should point to the corresponding property documentation in the base class

The properties are private. I don't know what we should point to.

then we can duplicate the documentation here instead. I mainly want to have a definition of what "Contribution of the central atom required for LODE" means.

python/rascaline/examples/splined-radial-integral.py

Luthaf · 2023-09-20T12:11:06Z

python/rascaline/rascaline/utils/splines.py

+import numpy as np
+
+
+class SplinerBase(ABC):


since this is specifically for tabulated radial integral, I think it should be reflected somewhere in the name:

Suggested change

class SplinerBase(ABC):

class RadialIntegralSplinerBase(ABC):

Luthaf · 2023-09-20T12:11:26Z

python/rascaline/rascaline/utils/splines.py

+        return interpolated_values
+
+
+class SplinerFromFunction(SplinerBase):


Suggested change

class SplinerFromFunction(SplinerBase):

class RadialIntegralFromFunction(SplinerBase):

Luthaf · 2023-09-20T12:12:08Z

python/rascaline/rascaline/utils/splines.py

+        1-D array containing the values of the radial integral. :parameter cutoff:
+        Spherical cutoff for the radial basis :parameter max_angular: number of radial
+        components


Suggested change

1-D array containing the values of the radial integral. :parameter cutoff:

Spherical cutoff for the radial basis :parameter max_angular: number of radial

components

1-D array containing the values of the radial integral.

:parameter cutoff: Spherical cutoff for the radial basis

thanks. Always these issues with the auto formatting.

PicoCentauri · 2023-09-20T14:45:00Z

python/rascaline/rascaline/utils/splines.py

+
+        # The hardcoded displacement is a only a valid choice of the positions are in a
+        # reasonable range.
+        assert np.abs(positions).mean() >= 1.0


Should we do a propper raise here? Probably it is better...

would be nice yes =)

Luthaf

Looks good, once CI is happy we can merge!

Luthaf · 2023-09-21T13:07:12Z

python/rascaline/rascaline/utils/splines.py

+Classes for generating spline points which can be used as input for the for the radial
+integral hyper paramater option. For an complete example for the LE how to use the
+classes see :ref:`example-splines`.


Suggested change

Classes for generating spline points which can be used as input for the for the radial

integral hyper paramater option. For an complete example for the LE how to use the

classes see :ref:`example-splines`.

Classes for generating spline points which can be used as input for the radial

integral hyper parameter option. For an complete example on how to use the

classes see :ref:`example-splines`.

Luthaf · 2023-09-21T13:08:20Z

python/rascaline/rascaline/utils/splines.py

+    :members:
+    :show-inheritance:
+
+.. autoclass:: rascaline.utils.RadialIntegralFromFunction


we should pick a place for these. I see you like to have them in the module docstring, and I prefer them in the docs sources. Let's chat about it in the meeting later today?

Luthaf · 2023-09-21T13:09:33Z

python/rascaline/rascaline/utils/splines.py

+        radial basis function and :math:`n` the current radial channel. This should be
+        implemented in the child classes.


Suggested change

radial basis function and :math:`n` the current radial channel. This should be

implemented in the child classes.

radial basis function and :math:`n` the current radial channel. This should be

precomputed and provided separately.

Also allow to calculate the derivative of the radial integral numerically.

PicoCentauri requested a review from frostedoyster August 1, 2023 13:23

PicoCentauri force-pushed the splinerclass branch 2 times, most recently from 16bef0b to 0033cb2 Compare August 1, 2023 13:29

frostedoyster reviewed Aug 2, 2023

View reviewed changes

python/rascaline/tests/utils/splines.py Outdated Show resolved Hide resolved

python/rascaline/rascaline/utils/splines.py Show resolved Hide resolved

PicoCentauri force-pushed the splinerclass branch from 0033cb2 to 87ecb0c Compare August 3, 2023 08:08

agoscinski mentioned this pull request Aug 4, 2023

Integration of asv benchmark lab-cosmo/torch_spex#21

Open

PicoCentauri force-pushed the splinerclass branch 2 times, most recently from 3831432 to c41a737 Compare August 17, 2023 12:30

PicoCentauri force-pushed the splinerclass branch from c41a737 to c72b89b Compare August 31, 2023 11:39

PicoCentauri requested a review from Luthaf August 31, 2023 11:41

Luthaf force-pushed the splinerclass branch from c72b89b to bf415ce Compare September 1, 2023 10:44

PicoCentauri force-pushed the splinerclass branch 4 times, most recently from 8ef0ad4 to 5d6db7a Compare September 20, 2023 11:42

Luthaf reviewed Sep 20, 2023

View reviewed changes

Add flake8-sphinx-links to the linter

62d4081

PicoCentauri force-pushed the splinerclass branch from 5d6db7a to 9938d12 Compare September 20, 2023 14:44

PicoCentauri commented Sep 20, 2023

View reviewed changes

PicoCentauri force-pushed the splinerclass branch 2 times, most recently from 201c48d to 7299d88 Compare September 21, 2023 13:08

Luthaf approved these changes Sep 21, 2023

View reviewed changes

PicoCentauri force-pushed the splinerclass branch from 7299d88 to fa37ad1 Compare September 21, 2023 13:21

PicoCentauri added 3 commits September 21, 2023 15:51

Bump Python version in coverage CI

e72ffec

Added __all__ list in __init__.py

df16c2d

Replace generate_splines by a more flexible class

c835ea8

Also allow to calculate the derivative of the radial integral numerically.

PicoCentauri force-pushed the splinerclass branch from fa37ad1 to c835ea8 Compare September 21, 2023 13:52

PicoCentauri merged commit dea662e into master Sep 21, 2023
24 checks passed

PicoCentauri deleted the splinerclass branch September 21, 2023 15:02

Luthaf mentioned this pull request Sep 21, 2023

Small docs changes in splines #233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General spliner class #212

General spliner class #212

PicoCentauri commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading

frostedoyster left a comment

PicoCentauri commented Aug 31, 2023

Luthaf left a comment

Luthaf Sep 20, 2023

PicoCentauri Sep 20, 2023

Luthaf Sep 20, 2023

PicoCentauri Sep 20, 2023

Luthaf Sep 20, 2023

Luthaf Sep 20, 2023

Luthaf Sep 20, 2023

Luthaf Sep 20, 2023

PicoCentauri Sep 20, 2023

PicoCentauri Sep 20, 2023

Luthaf Sep 20, 2023

Luthaf left a comment

Luthaf Sep 21, 2023

Luthaf Sep 21, 2023

Luthaf Sep 21, 2023

		:parameter center_contribution: Contribution of the central atom required for LODE
		calculations.

	class SplinerBase(ABC):
	class RadialIntegralSplinerBase(ABC):

		return interpolated_values


		class SplinerFromFunction(SplinerBase):

	class SplinerFromFunction(SplinerBase):
	class RadialIntegralFromFunction(SplinerBase):

		radial basis function and :math:`n` the current radial channel. This should be
		implemented in the child classes.

General spliner class #212

General spliner class #212

Conversation

PicoCentauri commented Aug 1, 2023 • edited Loading

github-actions bot commented Aug 1, 2023 • edited Loading

frostedoyster left a comment

Choose a reason for hiding this comment

PicoCentauri commented Aug 31, 2023

Luthaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luthaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicoCentauri commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading