Revamped tuning #130

GardevoirX · 2024-12-17T16:12:49Z

This PR introduces two things:

A new tuning scheme based on grid-search (see Grid-searching based tuning scheme #110)
A new example by @ceriottm on the tuning (10-tuning)

Still more works need to be done, like writing documentations, fixing the pytests and the example, before this PR is ready.

Contributor (creator of pull-request) checklist

Tests updated (for new features and bugfixes)?
Documentation updated (for new features)?
Issue referenced (for PRs that solve an issue)?

Reviewer checklist

CHANGELOG updated with public API or any other important changes?

📚 Documentation preview 📚: https://torch-pme--130.org.readthedocs.build/en/130/

PicoCentauri · 2024-12-18T10:50:39Z

src/torchpme/utils/tuning/grid_search.py

+def grid_search(
+    method: str,
+    charges: torch.Tensor,


I would turn the logic around and keep the tune_XXX method. Also, grid_search is a very common name. It is not really clear from this that this will find the optimal parameters for the methods.

PicoCentauri · 2024-12-20T21:04:04Z

examples/05-autograd-demo.py

@@ -515,3 +518,82 @@ def forward(self, positions, cell, charges):
 print(f"Evaluation time:\nPytorch: {time_python}ms\nJitted:  {time_jit}ms")

 # %%
+# Other auto-differentiation ideas


IMHO opinion I wouldn't put this example here - even though I think it is good to have it. The tutorial is already 500 lines and with this super long. I rather vote for smaller examples tackling one specific tasks. Finding solutions is much easier if they are shorter. See also the beloved matplotlib examples.

docs/src/references/utils/tuning.rst

PicoCentauri

Why are there three Tuning base classes

TuningErrorBounds, TuningTimings which are hardcoded inside GridSearchBase? I think a single base class is enough, no?

src/torchpme/utils/tuning/__init__.py

PicoCentauri · 2025-01-07T20:52:21Z

src/torchpme/utils/tuning/p3m.py

+    CalculatorClass = P3MCalculator
+    GridSearchParams = {
+        "interpolation_nodes": [2, 3, 4, 5],
+        "mesh_spacing": 1 / ((np.exp2(np.arange(2, 8)) - 1) / 2),


Shouldn't we give the users the option to choose possibility to give the grid points on which they want to optimize.

This is mainly because the possible grid points were hard-coded before. If we can do this, would be good. We let the user input a list of their desired mesh_spacing at the beginning?

PicoCentauri · 2025-01-07T20:53:45Z

src/torchpme/utils/tuning/__init__.py

+            )
+            value = result.sum()
+            if self._run_backward:
+                value.backward(retain_graph=True)


why do you need to retain the graph here?

PicoCentauri · 2025-01-07T20:56:03Z

src/torchpme/utils/tuning/__init__.py

+                positions.requires_grad_(True)
+                cell.requires_grad_(True)
+                charges.requires_grad_(True)
+            execution_time -= time.time()


This looks a very weird way of storing the result. why not using a temp variable?

Suggested change

execution_time -= time.time()

t0 = time.time()

See below.

PicoCentauri · 2025-01-07T20:56:23Z

src/torchpme/utils/tuning/__init__.py

+
+            if self._device is torch.device("cuda"):
+                torch.cuda.synchronize()
+            execution_time += time.time()


Suggested change

execution_time += time.time()

execution_time += t0 - time.time()

PicoCentauri · 2025-01-07T20:58:39Z

src/torchpme/utils/tuning/__init__.py

+        self._charges = charges
+        self._cell = cell
+        self._positions = positions
+        self._dtype = charges.dtype
+        self._device = charges.device
+        self._n_repeat = n_repeat
+        self._n_warmup = n_warmup
+        self._run_backward = run_backward


Do you really need all of these private properties?

Many of these seem to be only used once and are hardcoded.

Also I think user variables should be stored public.

If I pass positions I should be able to access them via self.positions and not as a private property.

src/torchpme/utils/tuning/__init__.py

PicoCentauri · 2025-01-07T21:00:08Z

examples/10-tuning.py

+import ase
+import matplotlib as mpl
+import matplotlib.pyplot as plt
+import numpy as np


Shouldn't we try to be pure torch or is there a reason why we need numpy in this example.

PicoCentauri · 2025-01-09T08:48:10Z

Some notes from our meeting

give List[Dict] and class to the init of the tuner class where the dict can be a named or unnamed parameters passed to the init the calculator class.
class returns a List[floats] where floats are the obtained timings
user facing function i.e. tune_ewald will create the grid and returns the best parameters

PicoCentauri

Update docstrings and tests

write a doc page API page explaining the base class and how we do the tuning (Reuse the text for updating the paper). In the API references I would do a new section tuning. On the tuning page I would explain how we do the tuning. Then create one page for each calculator and finally one page for the base classes. One the base class page you explain how you designed these classes and how they work together.

The subpages for each calculator should first display the tuning function and below the classes for the error bounds. In the introduction text of each display the equation for error bounds.

PicoCentauri · 2025-01-15T11:05:47Z

src/torchpme/tuning/base.py

+        positions: torch.Tensor,
+        cutoff: float,
+        calculator,
+        params: list[dict],


I think you don't need the exponent. Should be able to extract it from the calculator.

but the Potential does not necessarily have the attribute exponent, like CoulombPotential 🤔

src/torchpme/tuning/base.py

PicoCentauri · 2025-01-15T11:08:02Z

src/torchpme/tuning/base.py

+        self._dtype = cell.dtype
+        self._device = cell.device
+


Should we put the dtype here as argument or deduce it from the claculator.

What do you say @E-Rum ?

PicoCentauri · 2025-01-15T11:09:33Z

src/torchpme/tuning/base.py

+    @staticmethod
+    def _validate_parameters(


This is now very similar to the one we use in the calculators, right?

Maybe we extact merge both and make them an standalone private function living in utils.

They are still slightly different from each other. The one in the calculators checks smearing while the one of tuning checks exponent, but it is possible to extract the common part

Okay yeah might be useful to have some code sharing if possible.

After getting the standalone functions, do we only call it in the tuning functions, or we still keep it being called during the initialization of the tuner?

src/torchpme/tuning/error_bounds.py

src/torchpme/tuning/tuner.py

src/torchpme/tuning/base.py

PicoCentauri

Yes I am happy with the design. I left some initial comments but we can start making the code ready to go in.

docs/src/references/index.rst

docs/src/references/tuning/base_classes.rst

docs/src/references/tuning/tune_ewald.rst

src/torchpme/tuning/error_bounds.py

…squares

fix

GardevoirX linked an issue Dec 17, 2024 that may be closed by this pull request

Grid-searching based tuning scheme #110

Open

PicoCentauri reviewed Dec 18, 2024

View reviewed changes

ceriottm force-pushed the revamped-tuning branch from 367f147 to f554b70 Compare December 20, 2024 07:58

PicoCentauri reviewed Dec 20, 2024

View reviewed changes

PicoCentauri reviewed Jan 3, 2025

View reviewed changes

docs/src/references/utils/tuning.rst Outdated Show resolved Hide resolved

GardevoirX marked this pull request as ready for review January 7, 2025 12:57

PicoCentauri reviewed Jan 7, 2025

View reviewed changes

GardevoirX force-pushed the revamped-tuning branch 2 times, most recently from a41f780 to 33c9705 Compare January 7, 2025 22:18

PicoCentauri reviewed Jan 15, 2025

View reviewed changes

src/torchpme/tuning/error_bounds.py Outdated Show resolved Hide resolved

PicoCentauri reviewed Jan 15, 2025

View reviewed changes

src/torchpme/tuning/tuner.py Outdated Show resolved Hide resolved

PicoCentauri reviewed Jan 15, 2025

View reviewed changes

src/torchpme/tuning/base.py Outdated Show resolved Hide resolved

PicoCentauri reviewed Jan 15, 2025

View reviewed changes

GardevoirX force-pushed the revamped-tuning branch from 250d8c6 to 135e6d9 Compare January 15, 2025 23:19

PicoCentauri reviewed Jan 16, 2025

View reviewed changes

docs/src/references/index.rst Outdated Show resolved Hide resolved

docs/src/references/tuning/base_classes.rst Show resolved Hide resolved

docs/src/references/tuning/tune_ewald.rst Show resolved Hide resolved

src/torchpme/tuning/error_bounds.py Outdated Show resolved Hide resolved

GardevoirX and others added 13 commits January 16, 2025 20:19

Initial version of grid_search

5b47f0c

Remove error

5936ccc

Allow a precomputed nl

6b8eba1

Renamed examples, and added a tuning playground

78e2a30

Nelder mead (doesn't work because actual error is not a good target)

96e8552

Added a tuning class

886d8d8

I'm not a morning person it seems

c5f0e66

Examples

2047ca2

Better plotting

4422082

Fixes on H and RMS_phi

abca208

Some cleaning and test fix

376c647

Further clean

adb25af

Replace loss in tuning with ErrorBounds and draft for Tuner

fcd155e

GardevoirX and others added 28 commits January 16, 2025 20:19

Documentations and pytests update

104d9c1

Added a TIP4P example

14c10c1

Started to change the API to use full charges rather than the sum of …

ab6fd50

…squares

Move from sum_squared_charges to charges

e0f7291

Refactor the tuning methods with a base class

567acdf

Fix pytests and make linter happy

9eaac38

Mini cleanups

6bca873

Docs fix

83532a3

Separate timings calculator

22cfa3e

Linting

47c154d

Try fix github action failures

bc831d3

Add tuning functions back

0be1c88

Allow doctests

d7a3165

Fix doctests and remove orphan functions

848dfd1

fix

Fix ewald doctest again and remove unused members

c813435

Formatting

949b94a

Draft for renovated tuning

9d43fb6

For now move back to CoulombPotential

2a2bf0f

Rearange the tuning stuff

4da66b4

Rearrange again

8021c35

An initial version of refurnished documentation

2ce197d

Minor modification

6c7b593

ErrorBounds related updates

d5b708a

Update error formulas

625c4f1

Update tuning tests

7044024

Lint manually

0019092

Update tuning doctests

0a83bbb

Update example

4c22071

GardevoirX force-pushed the revamped-tuning branch from d8e675e to 4c22071 Compare January 16, 2025 19:19

some minor cleanup

d55c9a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamped tuning #130

Revamped tuning #130

GardevoirX commented Dec 17, 2024 •

edited by github-actions bot

Loading

PicoCentauri Dec 18, 2024

PicoCentauri Dec 20, 2024

PicoCentauri left a comment

PicoCentauri Jan 7, 2025

GardevoirX Jan 7, 2025

PicoCentauri Jan 7, 2025

PicoCentauri Jan 7, 2025

PicoCentauri Jan 7, 2025

PicoCentauri Jan 7, 2025

PicoCentauri Jan 7, 2025

PicoCentauri commented Jan 9, 2025

PicoCentauri left a comment

PicoCentauri Jan 15, 2025

GardevoirX Jan 15, 2025

PicoCentauri Jan 15, 2025

PicoCentauri Jan 15, 2025

GardevoirX Jan 15, 2025

PicoCentauri Jan 15, 2025

GardevoirX Jan 16, 2025

PicoCentauri left a comment

	execution_time += time.time()
	execution_time += t0 - time.time()

Revamped tuning #130

Are you sure you want to change the base?

Revamped tuning #130

Conversation

GardevoirX commented Dec 17, 2024 • edited by github-actions bot Loading

Contributor (creator of pull-request) checklist

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicoCentauri 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

PicoCentauri commented Jan 9, 2025

PicoCentauri 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

PicoCentauri left a comment

Choose a reason for hiding this comment

GardevoirX commented Dec 17, 2024 •

edited by github-actions bot

Loading