search

TRIQS / tprf

TPRF: The Two-Particle Response Function tool box for TRIQS
https://triqs.github.io/tprf
Other
14 stars 12 forks source link

Eliashberg 3.0.x #15

Closed Stefan-Dienst closed 3 years ago

Stefan-Dienst commented 4 years ago

I have updated the eliashberg branch to a version I am happy with and is compatible with TRIQS 3.0.0. The changes include:

Stefan-Dienst commented 4 years ago

The build / build (macos-10.15, gcc-10, g++-10) fails 2 TPRF tests because matplotlib is missing: ModuleNotFoundError: No module named 'matplotlib'. But isn't matplotlib a dependency of TRIQS?

Wentzell commented 3 years ago

Dear @StefanKaeser,

First of all thank you for this major contribution!

Regarding the test-failure, matplotlib is currently just an optional dependency of TRIQS and is thus not installed in the test environments. Could you add a new line to the requirements.txt file containing the word matplotlib?

Stefan-Dienst commented 3 years ago

I am back from vacation. Thank you very much for adding the matplotlib requirement, Nils.

HugoStrand commented 3 years ago

Hi @StefanKaeser !

Thank you for porting and preparing your Eliashberg work. We can talk about the details in person, but here is a "wish list" of things.

Matplotlib requirement

Is it possible to get the code building without matplotlib?

I think it would be worth the effort not requiring that python module in the build process. It is a pain in the neck to install on some cluster systems, and give users trouble. If we just can make it an optional dependency it would be fantastic.

Speed up tests?

Is it possible to reduce/simplify the dimensions, increase temperature, etc. so that the eliasberg tests run a bit faster?

23/30 Test #23: Py_eliashberg-solve_eliashberg_functionality ......   Passed    4.77 sec
25/30 Test #25: Py_eliashberg-product_summation_vs_fft ............   Passed   11.15 sec
26/30 Test #26: Py_eliashberg-eigenvalue_solver ...................   Passed   16.41 sec
27/30 Test #27: Py_eliashberg-previous_implementation .............   Passed   20.06 sec
28/30 Test #28: Py_eliashberg-previous_implementation_two_band ....   Passed    3.21 sec
29/30 Test #29: Py_eliashberg-fft_product_constant_vs_full ........   Passed    1.13 sec
30/30 Test #30: Py_eliashberg-symmetrize_delta ....................   Passed   11.14 sec

eliashberg-previous_implementation and eliashberg-previous_implementation_two_band

This looks like a regression tests using reference data. Could we maybe rename them to "eliashberg_regression_single_band" etc. ?

Dummy.py cleaning

It seems like there are some testing python modules and tests that do nothing.

tprf/python/triqs_tprf/Dummy.py
tprf/test/python/Dummy.py
tprf/test/python/Dummy.out.h5

I think we could clean these up.

Copyright notices

I think my contribution both to the c++ code, python code, and the c++/python tests in the eliashberg part were very minor. Could you please put yourself as copyright holder and first author in the file headers? As it stands right now it looks as if I were the main author which I think is wrong (and the tests seems not to have headers).

Stefan-Dienst commented 3 years ago

Dear @HugoStrand,

thank you very much for your feedback. I have addressed all your points in the last few commits.

Unfortunately, now the build / build (ubuntu-20.04, gcc-10, g++-10) (pull_request) fails at the Install ubuntu dependencies step with the following error

Some packages could not be installed. This may mean that you have
requested an impossible situation or if you are using the unstable
distribution that some required packages have not yet been created
or been moved out of Incoming.
The following information may help to resolve the situation:

The following packages have unmet dependencies:
 libclang-10-dev : Depends: libclang1-10 (= 1:10.0.0-4ubuntu1) but 1:10.0.1~++20200708122807+ef32c611aa2-1~exp1~20200707223407.61 is to be installed
                   Depends: libclang-common-10-dev (= 1:10.0.0-4ubuntu1) but 1:10.0.1~++20200708122807+ef32c611aa2-1~exp1~20200707223407.61 is to be installed
Error: Process completed with exit code 100.

I have no idea why this now happens as I think, that my last commits can not influence this.

Wentzell commented 3 years ago

Dear @StefanKaeser,

The failure is related to a problem in the github ubuntu 20.04 image. You can ignore the failure for this PR.

HugoStrand commented 3 years ago

Dear @StefanKaeser,

Thank you for the fixes. Awesome work on tuning the tests!

21/30 Test #21: Py_eliashberg-preprocessing_gamma .................   Passed    1.03 sec
22/30 Test #22: Py_eliashberg-gamma_creation ......................   Passed    0.71 sec
23/30 Test #23: Py_eliashberg-solve_eliashberg_functionality ......   Passed    0.86 sec
24/30 Test #24: Py_eliashberg-semi_random_initial_delta ...........   Passed    0.74 sec
25/30 Test #25: Py_eliashberg-product_summation_vs_fft ............   Passed    1.23 sec
26/30 Test #26: Py_eliashberg-eigenvalue_solver ...................   Passed    3.14 sec
27/30 Test #27: Py_eliashberg-regression_test_one_band ............   Passed    1.57 sec
28/30 Test #28: Py_eliashberg-regression_test_two_band ............   Passed    1.96 sec
29/30 Test #29: Py_eliashberg-fft_product_constant_vs_full ........   Passed    0.83 sec
30/30 Test #30: Py_eliashberg-symmetrize_delta ....................   Passed    1.63 sec
HugoStrand commented 3 years ago

Dear @StefanKaeser,

I have finally gotten the building of the documentation working. It looks good overall.

However, in the Eliashberg theory section there is a scary note about a missing factor of 1/2.

foo

Is that something that has been solved? Could you please update the docs on this point?

Apart from this I think your branch is ready for merging!

Cheers, Hugo

Stefan-Dienst commented 3 years ago

Dear @HugoStrand

I have finally gotten the building of the documentation working. It looks good overall.

However, in the Eliashberg theory section there is a scary note about a missing factor of 1/2.

foo

Is that something that has been solved? Could you please update the docs on this point?

Thanks for pointing this out, I forgot to address this. But I have now fixed this inconsistency and adjusted to the TPRF definition with my latest commit. It turned out, that two of the Refs I mainly followed defined the particle-particle BSE without the factor 1/2, but added this factor inside the definition of the bare particle-particle susceptibility. I added this as a note and now everything agrees.

Cheers Stefan

HugoStrand commented 3 years ago

The tests are now passing. So I am merging this into unstable. Thank you @StefanKaeser for contributing! 🥇