i-PI integration #307

max-veit · 2020-12-11T18:23:48Z

Add support for i-PI using a dedicated calculator, which is also usable as a "generic" MD interface for any other MD code. Works with the branch https://github.com/cosmo-epfl/i-pi/tree/feat/librascal

The new calculator is implemented in bindings/rascal/models/genericmd.py.

Still to be worked out:

~~Confirm sign convention of virial~~ Confirmed experimentally
~~Make units handling more robust (fewer assumptions)~~ moved to i-PI
~~Does i-PI run non-periodic simulations? If so, how is this signalled to the force driver?~~ No

Tasks before review:

Co-written with Lorenzo (@lgigli)

bindings/rascal/models/krr.py

max-veit · 2021-02-25T15:55:51Z

bindings/rascal/models/krr.py

+def compute_KNM(frames, X_sparse, kernel, soap):
+    Nstructures, Ngrads, Ngrad_stride = get_strides(frames)
+    KNM = np.zeros((Nstructures + Ngrads, X_sparse.size()))
+    pbar = tqdm(frames, desc="compute KNM", leave=False)


In fact, I'm not sure how I feel about including tqdm at such a deep level -- it seems wrong for the code to need to know about something that's in principle unrelated, but I'm not sure I see a better solution.

One idea would instead be to wrap frames in a tqdm before passing it to this function in whatever notebook uses these - will look into this later.

will look into this later.

Did you try this? I would find it much more elegant, and it gives the user more control over the tqdm display name/positions/style etc.

Not yet; I ended up just using the fix in cf0e660.

Ok, I found a way that works -- it's not pretty, because the function iterates through the frames twice and I can't find a way to reset a tqdm bar once it's finished (you'd think that would be a pretty basic feature, but whatever).

bindings/rascal/models/krr.py

Luthaf · 2021-03-25T09:28:16Z

A few high level comments before I dive into this for a review

could you fix conflicts with master?
do we really need an additional 11Mb file reference_data/tests_only/simple_gap_fit_params.json just for the tests? Could this be made much smaller by using less sparse points when training? We don't need an accurate potential just to check that the machinery around it works. Related to this, could you then remove the file completely from history by rebasing this branch or squashing the corresponding commits?
where is the format for reference_data/tests_only/simple_gap_fit_params.json defined and documented? My current understanding is that this file contains the raw JSON serialization of all C++ classes involved in the model, plus the weights and sparse points, am I right? I'm afraid to start distributing / encourage users to generate and distribute models with an under-defined and un-documented file format. Do we want to guarantee support for this format? Or should people re-train their models whenever we change librascal internals? Overall I feel that the format we want to use to store trained models warrant a larger discussion than on the side of semi-related PR.

max-veit · 2021-03-25T10:22:47Z

could you fix conflicts with master?

I'll do that when I rebase this branch. The (tiny) conflicts here were caused by a rebase/squash of yesterday's bugfix branch (that was extracted from this one).

do we really need an additional 11Mb file reference_data/tests_only/simple_gap_fit_params.json just for the tests? Could this be made much smaller by using less sparse points when training? We don't need an accurate potential just to check that the machinery around it works. Related to this, could you then remove the file completely from history by rebasing this branch or squashing the corresponding commits?

Didn't realize it was that large... I was already using what felt like a very small number of sparse points, but I'll try to cut it down further.

where is the format for reference_data/tests_only/simple_gap_fit_params.json defined and documented? My current understanding is that this file contains the raw JSON serialization of all C++ classes involved in the model, plus the weights and sparse points, am I right? I'm afraid to start distributing / encourage users to generate and distribute models with an under-defined and un-documented file format. Do we want to guarantee support for this format? Or should people re-train their models whenever we change librascal internals? Overall I feel that the format we want to use to store trained models warrant a larger discussion than on the side of semi-related PR.

That's really a discussion for #305 (which is going to be my main "project" after this PR; it was also the branch used to generate this model file). To summarize for now, the model JSON is the serialization of a Python KRR object, which can be restored (via the librascal deserialization mechanism) and used to evaluate the model via the public KRR interface.
It does contain the serialized representation and kernel parameters, as well as the weights and sparse points -- but as to the concern about model files changing every time we change something in the internals of librascal, that seems unlikely as the serialization happens on a relatively high level and only uses the Python interface.

And anyway, the file lives in reference_data/*tests_only*, not examples/, so I hope that would be enough of an indication for users not to take this as a definitive model specification. The only point to having this model file right now is to be able to test the i-PI interface.

(still need to work out some kinks in the return format, depending on what i-PI expects)

and fix naming issue

also update description and autoformat

Simplify and test IPI tutorial notebook; replace BaTiO3 example with Zundel

Remove i-PI specific unit conversions and input/output formatting; this should be taken care of on the i-PI side.

Edit: reduce the size of simple testing GAP model file

Cherry-picked from feat/gaptools for compatibility with gaptools-generated model

(although probably they should be moved to something like a notebook-utils folder, or eventually just merged into gaptools) also, remove unused imports in zundel i-PI example

and update versions in requirements.txt too (for CI)

README.rst

bindings/rascal/models/IP_generic_md.py

examples/iPi/zundel/zundel_IP.ipynb

Luthaf · 2021-03-26T13:38:36Z

The updated example model file is much better, and I'm fine with leaving the discussion of the serialization format for models for a later PR!

- Remove 'assume_pbc' option; user must now specify PBC explicitly on initialization - Allow specifying bare 'atomic_numbers' in lieu of structure template - Check for number-of-atoms consistency

It should only ever be initialized via the standard __init__ way

max-veit · 2021-03-29T14:15:25Z

I'm pretty much done on the bindings side; perhaps @lgigli can have a look at the notebook comments since he's the one who wrote it?

max-veit · 2021-04-08T20:56:34Z

not sure what's happening with the doc build... Sphinx is complaining about some utf-8 characters in a file it's trying to interpret in ASCII mode (why??? why won't ASCII die already??) -- but I'm pretty sure I didn't add any funny characters or change file encodings or the build configuration since the last successful doc build. And it runs just fine on my machine.

bindings/rascal/models/genericmd.py

max-veit · 2021-04-09T09:29:46Z

not sure what's happening with the doc build... Sphinx is complaining about some utf-8 characters in a file it's trying to interpret in ASCII mode (why??? why won't ASCII die already??) -- but I'm pretty sure I didn't add any funny characters or change file encodings or the build configuration since the last successful doc build. And it runs just fine on my machine.

This is full (!) traceback of the error, looks like it happens when trying to load the ExtBabel extension for some reason. Why this is failing now and not in earlier builds, I have no idea.

# Sphinx version: 3.5.3
# Python version: 3.6.9 (CPython)
# Docutils version: 0.17 release
# Jinja2 version: 2.11.3
# Last messages:

# Loaded extensions:
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/sphinx/cmd/build.py", line 279, in build_main
    args.tags, args.verbosity, args.jobs, args.keep_going)
  File "/usr/local/lib/python3.6/dist-packages/sphinx/application.py", line 241, in __init__
    self.setup_extension(extension)
  File "/usr/local/lib/python3.6/dist-packages/sphinx/application.py", line 402, in setup_extension
    self.registry.load_extension(self, extname)
  File "/usr/local/lib/python3.6/dist-packages/sphinx/registry.py", line 417, in load_extension
    mod = import_module(extname)
  File "/usr/lib/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/usr/local/lib/python3.6/dist-packages/sphinx/builders/latex/__init__.py", line 25, in <module>
    from sphinx.builders.latex.util import ExtBabel
  File "/usr/local/lib/python3.6/dist-packages/sphinx/builders/latex/util.py", line 13, in <module>
    from docutils.writers.latex2e import Babel
  File "/usr/local/lib/python3.6/dist-packages/docutils/writers/latex2e/__init__.py", line 575, in <module>
    for line in fp:
  File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 69: ordinal not in range(128)

max-veit · 2021-04-09T10:02:33Z

omg no way, the build is failing due to a bug in the latest version (0.17) of docutils. Specifically, the '©' character in the copyright notice of docutils.sty... already reported here and fixed in the latest Sourceforge version, but for us I think we can just force the previous version in requirements.txt until a hotfix is pushed to pypi.

Luthaf

The code looks good, feel free to merge after squashing related commits together!

examples/i-PI/zundel/zundel_IP.ipynb

Note also: i-PI examples dir has been renamed Co-authored-by: Guillaume Fraux <[email protected]>

(https://sourceforge.net/p/docutils/bugs/414/, can revert this once it's pushed to pyPI)

max-veit commented Feb 25, 2021

View reviewed changes

bindings/rascal/models/krr.py Outdated Show resolved Hide resolved

max-veit marked this pull request as ready for review February 25, 2021 15:42

max-veit commented Feb 25, 2021

View reviewed changes

Luthaf mentioned this pull request Mar 23, 2021

Quick fix for circular imports #325

Merged

max-veit commented Mar 24, 2021

View reviewed changes

bindings/rascal/models/krr.py Show resolved Hide resolved

max-veit commented Mar 24, 2021

View reviewed changes

bindings/rascal/models/krr.py Outdated Show resolved Hide resolved

max-veit requested review from agoscinski, ceriottm and Luthaf March 24, 2021 16:27

ceriottm and others added 19 commits March 26, 2021 12:37

Added a stub of an i-PI interface

bb424fa

Update initialization and storing of I-PI interface class

d1ae923

Partial implementation of i-PI compute function

0ebe0b1

(still need to work out some kinks in the return format, depending on what i-PI expects)

Add virial output in the format i-PI expects

02cf947

and fix naming issue

IT'S ALIIIIIIVE

ebcf075

Flip virial sign convention to match i-PI's

5bc3ffa

Add driver script

1dd6f41

Rename i-PI driver so it's clear what it drives

79e4557

also update description and autoformat

Add i-Pi example

0ef0e8c

Simplify and test IPI tutorial notebook; replace BaTiO3 example with Zundel

Make i-PI calculator into a generic ML-MD interface

1a898e1

Remove i-PI specific unit conversions and input/output formatting; this should be taken care of on the i-PI side.

Remove old, unused i-PI driver scripts

3a6852a

Make dependency on tqdm optional

cf0e660

Reorganize docs and make a place to discuss i-PI interface

68d6cad

Expand doc on MD interfaces

c1329c5

Add simple GAP model (and source data and params) for tests

547c922

Edit: reduce the size of simple testing GAP model file

Make generic MD read only the first structure of a given template file

0df18fc

Add annotations (with basic units) to gap model output file

7390c79

Cherry-picked from feat/gaptools for compatibility with gaptools-generated model

Add tests for GenericMDCalculator

737708f

Document extra utilities in krr.py

d4479cc

(although probably they should be moved to something like a notebook-utils folder, or eventually just merged into gaptools) also, remove unused imports in zundel i-PI example

max-veit added 2 commits March 26, 2021 13:08

Add Zundel i-PI example to online docs

384ae15

Update version requirements for compiling docs

d2fd3a9

and update versions in requirements.txt too (for CI)

max-veit force-pushed the feat/ipi branch from d19f761 to d2fd3a9 Compare March 26, 2021 12:10

Luthaf reviewed Mar 26, 2021

View reviewed changes

This was referenced Mar 26, 2021

Dependencies list is duplicated between documentation and requirements.txt #327

Open

structure sanitation #323

Open

max-veit added 2 commits March 29, 2021 14:06

Rename models/IP*.py to shorter, more Pythonic module names

9027d9e

Overhaul initialization of generic MD calculator

02e2323

- Remove 'assume_pbc' option; user must now specify PBC explicitly on initialization - Allow specifying bare 'atomic_numbers' in lieu of structure template - Check for number-of-atoms consistency

max-veit added a commit to lab-cosmo/i-pi that referenced this pull request Mar 29, 2021

Update rascal driver with upstream changes (lab-cosmo/librascal#307)

08fde53

max-veit added 3 commits March 29, 2021 14:39

Remove unneeded serialization of generic MD driver

264972a

It should only ever be initialized via the standard __init__ way

Update KRR utils doc

9b529e3

Add tests for new GenericMD initialization

3281e07

max-veit requested a review from Luthaf April 8, 2021 15:54

Luthaf reviewed Apr 9, 2021

View reviewed changes

bindings/rascal/models/genericmd.py Show resolved Hide resolved

Luthaf approved these changes Apr 12, 2021

View reviewed changes

examples/i-PI/zundel/zundel_IP.ipynb Outdated Show resolved Hide resolved

max-veit and others added 2 commits April 12, 2021 16:15

Update example notebook, remove tqdm dependency from bindings

8a074b3

Note also: i-PI examples dir has been renamed Co-authored-by: Guillaume Fraux <[email protected]>

Force docutils version to avoid bug in 0.17

6a9f4bd

(https://sourceforge.net/p/docutils/bugs/414/, can revert this once it's pushed to pyPI)

max-veit force-pushed the feat/ipi branch from bc37d15 to 6a9f4bd Compare April 12, 2021 14:15

max-veit merged commit 060126b into master Apr 12, 2021

max-veit deleted the feat/ipi branch April 12, 2021 14:53

max-veit mentioned this pull request Apr 12, 2021

Interface to use librascal potentials through driver.py i-pi/i-pi#171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i-PI integration #307

i-PI integration #307

max-veit commented Dec 11, 2020 •

edited

Loading

max-veit Feb 25, 2021

Luthaf Mar 26, 2021

max-veit Mar 26, 2021

max-veit Apr 8, 2021

Luthaf commented Mar 25, 2021

max-veit commented Mar 25, 2021 •

edited

Loading

Luthaf commented Mar 26, 2021

max-veit commented Mar 29, 2021

max-veit commented Apr 8, 2021

max-veit commented Apr 9, 2021

max-veit commented Apr 9, 2021

Luthaf left a comment

i-PI integration #307

i-PI integration #307

Conversation

max-veit commented Dec 11, 2020 • edited Loading

max-veit Feb 25, 2021

Choose a reason for hiding this comment

Luthaf Mar 26, 2021

Choose a reason for hiding this comment

max-veit Mar 26, 2021

Choose a reason for hiding this comment

max-veit Apr 8, 2021

Choose a reason for hiding this comment

Luthaf commented Mar 25, 2021

max-veit commented Mar 25, 2021 • edited Loading

Luthaf commented Mar 26, 2021

max-veit commented Mar 29, 2021

max-veit commented Apr 8, 2021

max-veit commented Apr 9, 2021

max-veit commented Apr 9, 2021

Luthaf left a comment

Choose a reason for hiding this comment

max-veit commented Dec 11, 2020 •

edited

Loading

max-veit commented Mar 25, 2021 •

edited

Loading