Feat/kvec generator

[kvhuguenin]

Implemented class for generating the k-vectors (reciprocal space vectors) used for the k-space implementation of LODE and more general spherical expansion coefficients. The details and derivations of the algorithm used to generate the vectors is described in a supplementary document that can be provided if desired.

Tests are provided to check that the code behaves reasonably under realistic conditions as well as some more artificial tests to catch boundary cases.

[Luthaf]

The details and derivations of the algorithm used to generate the vectors is described in a supplementary document that can be provided if desired.

Could you summarize it in the documentation of Kvectors? Otherwise, it might be a good idea to add this to the librascal documentation.

[agoscinski]

So the pip installation has problems to find zlib (dependency matplotlib -> pillow -> zlib), so @kvhuguenin added apt-get install libjpeg-dev zlib1g-dev to the circle ci jobs before the pip installation as temporary solution. I think the better solution would be to update the docker images, but I do not have experience with this. Maybe @Luthaf do you have some input?

https://app.circleci.com/pipelines/github/cosmo-epfl/librascal/2735/workflows/fe45599f-b070-4204-9bb3-1d4e867098a3/jobs/24054

[Luthaf]

This is a bit of a strange behavior, I would expect us to get pre-built wheels instead of having pip rebuild whatever code is in Pillow. I would rather investigate in this direction before modifying the docker images.

If you look through the logs, the failing build contains two things saying it is building from source:

• "Collecting pillow>=6.2.0 [...] Downloading https://files.pythonhosted.org/[...]/Pillow-8.4.0.tar.gz (49.4MB)", where it is downloading a source distribution
• "Building wheels for collected packages: black, py-ubjson, pillow", when building the wheels

Previous builds that worked fine (https://app.circleci.com/pipelines/github/cosmo-epfl/librascal/2727/workflows/b8d9d737-fa46-45d4-9710-53eeed785e8e/jobs/23967) looked like this instead:

• "Collecting pillow>=6.2.0 [...] Downloading https://files.pythonhosted.org/[...]/Pillow-8.3.2-cp36-cp36m-manylinux_2_5_x86_64.manylinux1_x86_64.whl (3.0MB)", where it is downloading a wheel (.whl)

My best guess is that they just released Pillow 8.4.0, and either did not had time to publish wheels for Python 3.6, or decided to drop Python 3.6 support.

---

If we really need to modify them, the dockerfiles are in the repo (https://github.com/cosmo-epfl/librascal/blob/master/.circleci/Dockerfile), and I can give you access to the dockerhub organization so that you can publish new version of the image.

[kvhuguenin]

I have now updated and polished the code, the two important changes of which have been:

• the print_analysis() function that was only used by me during the testing phase to make sure that the results we obtain were meaningful has now been removed and turned into a test using rigorous mathematical bounds. There are now two simpler functions that just return some benchmark values for what to expect, whose values are used in the test. Thanks for @agoscinski for the input.
• I have added a description of the algorithm that covers all essential ideas of the derivation in the .cc file before the algorithm is actually used. Thank you for the comments @Luthaf . If this would make the code better, I wouldn't mind adding a full step by step derivation. However, in my Latex document, that takes >2 pages, so I don't really know if it makes sense, even in the documentation. That being said, I think that LODE will need its own page in the documentation and there, we should probably have a subsection on the kvecgen (level of detail up to you). I would prefer to do that by integrating it as part of the LODE and reciprocal space SOAP description (currently working on the tests for that so we can do a PR for that soon, btw), since that is probably the only way a user will be able to interact with the kvecgen in the foreseeable future. For me, writing a description of the kvecgen that needs to make sense without ever mentioning LODE feels a little unnatural, based on the #381 mantra. Would that work for you @Luthaf ?

[kvhuguenin]

I am confused about the circleci notebooks test that failed. It is related to the zundel notebook. When I pushed my changes 3 days ago (commit 22d6e0f), all tests passed. During the entire work on this branch, I haven't touched a single Python file, and the only files I have modified are related to the kvecgen which as of now, isn't used by any other code. I saw that the zundel notebook was also causing problems in #382, although in my case, it somehow seems like matplotlib couldn't get the right dependencies for Greek letters. Do you have any idea what could be the reason for this? (or could it be that the current issue is related to the underlying cause of #382?)

[Luthaf]

So documenting the user-side of LODE & kvecgen together is fine for me, and can be left for a future PR.

The failure of the Zundel notebook looks different from #382 (which is a timeout). Might be related to the problem we have with pip installing stuff from sources?

Talking about that, this error when installing Pillow is quite strange: they released wheels (as visible here https://pypi.org/project/Pillow/#files) on the 15th of october. I'll try to give this a look.

[kvhuguenin]

Talking about that, this error when installing Pillow is quite strange: they released wheels (as visible here https://pypi.org/project/Pillow/#files) on the 15th of october. I'll try to give this a look.

Ok, thanks for looking into that. Let me know if there is anything else I can do on this branch in the meantime. Otherwise, I will just keep working on the PRs for the remaining LODE packages that will come once this one has been merged (I still have to write the tests and polish everything, so it will take a few days).

[Luthaf]

Ok, now that #388 have been merged, could you rebase this branch on top of master?

[kvhuguenin]

Thank you for all the suggestions. I have now pushed a version that has implemented all these changes. The largest two of which are:

• all functions in the .hh file used only for tests was moved to the "kvecgen_numvectors_reasonable_test". The class now only contains the constructor + the three functions that will be called by LODE in its current implementation. It really is true that the class looks much cleaner now, so thank you @Luthaf for the suggestion!
• since we had some discussions regarding the rotation test, I have now made a new test using arbitrary rotation matrices by the method suggested by @agoscinski . When I ran the tests, I found that the sup/max/infinity-norm between the original and rotated vectors obtained could be as large as 4e-14 with a relatively small cutoff, so this combined with a very numerical error-prone way to implement rotation matrices originally might have led to my "bad luck" of failing rotation tests in the past.

[kvhuguenin]

Right now, for some reason I don't yet understand, the tests compiled with the clang compiler (that's what the clang-n tests are, right?) produce errors which seem to be due to the fact that the "rotation" matrices that are produced by the algorithm are not actual rotation matrices, i.e. they do not satisfy R^TR = RR^T=1. On my machine, the test runs fine. Is anybody familiar with this isse?

[agoscinski]

I am looking into this now

[kvhuguenin]

Oh nice! Thank you for the quick fix and lesseon @agoscinski ! I thought that using auto more might increase readability, but I should be more careful in the future then. Anyway, since this is the first time my code gets reviewed by somebody who actually knows how to do this, let me know if there is anything else that can be improved, both for this code specifically and also in terms of general programming practices @Luthaf and @agoscinski .

[agoscinski]

I am fine with the current state, just needs a rebase (or can be also squashed when merged, should be easier)