simplified and documented the lambda_lanczos_util.hpp file (again)

[jewettaij]

Forgive me for sending you my previous pull request.
(It was not yet tested. When I tried to use it, it did not compile.)

Here is the newest version of the lambda_lanczos_util.hpp file which I plan to include with LAMMPS. (This time I have tested it.)

Changes:

I was able to simplify the code for real_t and ConjugateProduct.

Feel free to ignore this pull request. I do not know if this is helpful. I worry I am creating too much work for you.

[jewettaij]

Unfortunately, I noticed that this version failed your automatic tests. (But it did not fail my tests. We have different tests.)

I noticed there are some additional differences between the "lambda_lanczos_util.hpp" file:

I deleted these lines from my version:

namespace lambda_lanczos_util {
namespace {
template<typename T>
using vector = std::vector<T>;

template<typename T>
using complex = std::complex<T>;
}

I deleted these lines from my version, but they are in your version.

For reason's I don't understand, my version compiles and runs with my test program.
To get it to work with your version, perhaps I should put these lines back. Alternately, we could replace vector with std::vector and complex->std::complex.

Again, feel free to ignore these suggestions.

I am not sure they are helpful.

Cheers!

[mrcdr]

Thank you for your suggestion.

About compile error

My test output implies that the std:: namespace modifier is needed, as you mentioned. Could you check whether you are putting something like using namespace std; somewhere on your test code or included header files?

When I wrote the lines (which are with an anonymous namespace) I thought they can prevent using-declaration leaking outside of the header file. (When using namespace lambda_lanczos_util is declared, the vector shouldn't be available without std::.) But they can't seem to do so and I can't find any proper way to use using-declaration on a namespace scope in a header file. To avoid the leak (or "pollution") of using declaration, we may have to remove the lines and place std:: before all of vector and complex.

Template specialization

I defined the template specialization for float, double and long double to inhibit use of some meaningless type like real_t<std::string> (which causes a compile error in current version). However they also inhibit some domain-specific floating-point like real_t<MyDouble>. Which is useful and easier to debug, to allow the template specialization for the other types or not? Incidentally, the std::complex<std::string> causes no compile error with g++. But it is indeed undefined behavior according to the C++ specification.

ConjugateProduct

Throughout my header files, class (structure) definitions and their implementation are separately written regardless of static or not. Strictly according to this rule, the implementation shouldn't be moved to inside class definition. However, it might be more readable that ConjugateProduct is defined at one place.

[jewettaij]

Thank you for your suggestion.

About compile error

My test output implies that the std:: namespace modifier is needed, as you mentioned. Could you check whether you are putting something like using namespace std; somewhere on your test code or included header files?

Yes. I am using the old version of "lambda_lanczos_test.cpp" (in the old "src/lambda_lanczos_test" subdirectory). The beginning of that file contains

using std::vector;
using std::complex;

To avoid the leak (or "pollution") of using declaration, we may have to remove the lines and place std:: before all of vector and complex.

In the LAMMPS version of LambdaLanczos, that is what I did.
In my opinion, this does not make the code look bad.
(But perhaps it is is avoidable...)

When I wrote the lines (which are with an anonymous namespace) I thought they can prevent using-declaration leaking outside of the header file. (When using namespace lambda_lanczos_util is declared, the vector shouldn't be available without std::.) But they can't seem to do so and I can't find any proper way to use using-declaration on a namespace scope in a header file.

I was confused by the current code. But now I understand why you used an anonymous namespace.

Why not replace:

namespace {
template<typename T>
using vector = std::vector<T>;

template<typename T>
using complex = std::complex<T>;
}

... with this?:

using std::vector;
using std::complex;

..or this?:

using namespace std;

Template specialization

I defined the template specialization for float, double and long double to inhibit use of some meaningless type like real_t<std::string> (which causes a compile error in current version). However they also inhibit some domain-specific floating-point like real_t<MyDouble>. Which is useful and easier to debug, to allow the template specialization for the other types or not?

It is your choice. I chose the shortest method. However I think both methods are equally clear as long as there is a good comment to explain what you mean by real_t<T>.

(I think real_t<T> is quite clever, by the way.)

Incidentally, the std::complex<std::string> causes no compile error with g++. But it is indeed undefined behavior according to the C++ specification.

ConjugateProduct

Throughout my header files, class (structure) definitions and their implementation are separately written regardless of static or not. Strictly according to this rule, the implementation shouldn't be moved to inside class definition. However, it might be more readable that ConjugateProduct is defined at one place.

That's fine. Be consistent. No reason to change this.

Cheers and thanks for your feedback.

Andrew

[mrcdr]

The use of using in a header file affects every cpp files which include the header file implicitly. The problem is that the user who includes the header file can't know the "using" is declared so some unexpected names could be introduced into the scope (some arguments are available on this).

It seems that the using is very controversial in C++. Google Style Guide says the using-directive (using namespace foo;) shouldn't be used even in cpp files.

[jewettaij]

I also prefer to avoid using.

To avoid the leak (or "pollution") of using declaration, we may have to remove the lines and place std:: before all of vector and complex

I think this is what I will do.
Cheers

Andrew

[mrcdr]

Thank you.

Yes, I think so too. I'll rewrite the source code to use the std:: qualifier.

P.S.
I'm so sorry to confuse you with the eigenvalue_offset. Currently I'm reviewing some articles to know more about mathematical aspects of the Lanczos algorithm. I can't say for certain, but some future version probably won't require the eigenvalue_offset (as mentioned as "mystery").
I really thank you for improving this library!