Refactor: Preconditioner function library #5482

maki49 · 2024-11-14T06:22:41Z

"precondition_funcs.h" is designed as a function library with following function types:

single-value transformer: none / qe-pw hard-coded formula)
vector-vector operator: div(prevec) / div(trans(prevec-eig)
(In the future, at one's need) Matrix-vector operator

Two ways to use this library:

Construct your own lambda / funciton with the library functions (or not), and pass it directly into the eigensolver (e.g. in module_lr)
Construct a PreOP object to preallocate the device memory at its construction, then use .get() interface to get the function to be called by the eigensolver (e.g. in hsolver_pw)

Now this library is applied to "dav" and "dav_subspace". The precondition method for "cg" and "bpcg" seems more complicated, so I will not change them before further investigation.

Cstandardlib · 2024-11-15T06:36:46Z

source/module_hsolver/diago_david.h

-  public:

-    DiagoDavid(const Real* precondition_in,
+    using PreFunc = fvec::Div<T>;


Is it possible to give PrecondFunc the interface like HPsi or SPsi?

// in precondition lib using Div = std::function<void(T*, const size_t&, const size_t&)>;

Maybe we can give it the form

using HPsiFunc = std::function<void(T*, T*, const int, const int)>; using SPsiFunc = std::function<void(T*, T*, const int, const int)>; using PrecndFunc = std::function<void(T*, T*, const int, const int)>; // maybe it's better to change int to size_t, I'll do it on current HPsiFunc and SPsiFunc

and pass the preconditioners to this universal function wrapper.

Sometimes the source and destination of preconditioning may be different. For example from a temporary block residual r to a block of total search space X[:, nband:nband+notconv]. The results will be directly stored to a new array in such case.

Good point, see the latest change.

And seems on HPsiFunc and SPsiFunc can "const" also be added like this:

using HPsiFunc = std::function<void(T*const, const T*const, const int, const int)>;

mohanchen · 2024-11-17T03:15:32Z

python/pyabacus/src/hsolver/py_diago_dav_subspace.hpp

            std::copy(hpsi_ptr, hpsi_ptr + nvec * ld_psi, hpsi_out);
        };

+        hsolver::PreOP<std::complex<double>, base_device::DEVICE_CPU, hsolver::fvec::DivTransMinusEigKernel<std::complex<double>, base_device::DEVICE_CPU>>


The name "DivTransMinusEigKernel" is a disaster.

Sorry but do you have any better idea?

The function should be named with following considerations: precisely describe what it does. In present case, you mix the mathematical operation with some rare numerical trick description like "kernel". So there will be quite few people can know what this function does from its name. You should either name the function purely with names mathematical operations, or use human natural language describe the action that what quantity is calculated, additionally from what quantity.

mohanchen · 2024-11-17T03:16:31Z

python/pyabacus/src/hsolver/py_diago_david.hpp

            syncmem_op()(this->ctx, this->ctx, spsi_out, psi_in, static_cast<size_t>(nbands * nrow));
        };

+        hsolver::PreOP<std::complex<double>> pre_op(precond_vec);


其它地方命名都是下划线命名法，PreOP这个名字起得能不能有点默契！

https://github.com/MCresearch/abacus-user-guide/blob/master/develop-C++.md
根据这个文档，变量名和命名空间用下划线命名法，类型名用的是每个单词首字母大写的命名法。

mohanchen · 2024-11-17T03:24:09Z

如果编程风格不改到我满意，我拒绝所有PR。

kirk0830 · 2024-11-17T04:28:21Z

I will carefully review this PR tomorrow.

kirk0830

I have reviewed only parts of this PR, there are still many lines I have not enough time to review, I will continue later.

kirk0830 · 2024-11-18T01:36:52Z

source/module_hsolver/precondition_funcs.h

+    namespace fval
+    {
+        template <typename T> T none(const T& x) { return x; }
+        template <typename T> T qe_pw(const T& x) { return 0.5 * (1.0 + x + sqrt(1 + (x - 1.0) * (x - 1.0))); }


I am a little bit confused about the name of this function, also I am curious if it can be accelerated automatically with SIMD, can you answer my question?

kirk0830 · 2024-11-18T01:42:36Z

source/module_hsolver/precondition_funcs.h

+        /// type 1: directly divide each vector by the precondition vector
+        ///---------------------------------------------------------------------------------------------
+        template <typename T, typename Device = base_device::DEVICE_CPU>
+        void div_prevec(T* const dst, const T* const src, const size_t& dim, const size_t& nvec,


it is not a good idea to use a term "prevec". Although it is scarcely seen the abbreviation of it directly or easily, at least you can know the word "condition" has abbr. "cond", so I think "precond" is okay. Then I will question "what is precondition vector"? is it a formal name of some numerical/technical operation? I guess not. Then I will ask you what is the meaning of "vector" in your code? Is it about the data structure? the way to store data which indicates the memory is continuous, or a mathematical description? Only the last one is acceptable.

kirk0830 · 2024-11-18T01:45:11Z

source/module_hsolver/precondition_funcs.h

+            for (size_t m = 0; m < nvec; m++)
+            {
+                const size_t offset = m * dim;
+                vector_div_vector_op<T, Device>()(ctx, dim, dst + offset, src + offset, pre);


actually the name vector_div_vector_op is not a good name, so please do not get any idea about naming functions from it. But you are not expected to improve its name in this PR

kirk0830 · 2024-11-18T01:49:36Z

python/pyabacus/src/hsolver/py_diago_dav_subspace.hpp

            std::copy(hpsi_ptr, hpsi_ptr + nvec * ld_psi, hpsi_out);
        };

+        hsolver::PreOP<std::complex<double>, base_device::DEVICE_CPU, hsolver::fvec::DivTransMinusEigKernel<std::complex<double>, base_device::DEVICE_CPU>>


I cannot see any significance of the necessarity of instantiating the hsolver::PreOP. The name of PreOP is also confusing quite much. From the rest of your PR, I can understand it is about preconditioning, but unfortunately you did not name them with much care and enough considerations.
You should spend at least 60% of your time on coding to carefully consider the name of variable, function, class, namespace, and the design of interface.

kirk0830 · 2024-11-18T01:51:13Z

python/pyabacus/src/hsolver/py_diago_dav_subspace.hpp


+        hsolver::PreOP<std::complex<double>, base_device::DEVICE_CPU, hsolver::fvec::DivTransMinusEigKernel<std::complex<double>, base_device::DEVICE_CPU>>
+            pre_op(precond_vec, hsolver::fvec::div_trans_prevec_minus_eigen<std::complex<double>>, hsolver::fval::qe_pw<double>);
        obj = std::make_unique<hsolver::Diago_DavSubspace<std::complex<double>, base_device::DEVICE_CPU>>(


it is the first time that I find there is an instance even named obj. Informativeless.

kirk0830 · 2024-11-18T01:59:03Z

python/pyabacus/src/hsolver/py_diago_dav_subspace.hpp

            std::copy(hpsi_ptr, hpsi_ptr + nvec * ld_psi, hpsi_out);
        };

+        hsolver::PreOP<std::complex<double>, base_device::DEVICE_CPU, hsolver::fvec::DivTransMinusEigKernel<std::complex<double>, base_device::DEVICE_CPU>>


presently the lines 133, 134 are quite confusing and it is quite difficult for others to know what the operation is. It wastes other's time. You should always consider a clearer way to organize your code like:

// instantiate hsolver::PreOP hsolver::PreOP<std::complex<double>, base_device::DEVICE_CPU, hsolver::fvec::DivTransMinusEigKernel< std::complex<double>, base_device::DEVICE_CPU> > pre_op(precond_vec, hsolver::fvec::div_trans_prevec_minus_eigen<std::complex<double>>, hsolver::fval::qe_pw<double>);

however, do you think naming the instance of PreOP as pre_op is a good idea?

kirk0830 · 2024-11-18T02:00:21Z

source/module_hsolver/diago_dav_subspace.cpp


 template <typename T, typename Device>
-Diago_DavSubspace<T, Device>::Diago_DavSubspace(const std::vector<Real>& precondition_in,
+Diago_DavSubspace<T, Device>::Diago_DavSubspace(PreFunc&& precondition_in,


could you explain the necessarity of using rvalue instead of lvalue?

kirk0830 · 2024-11-18T02:06:29Z

source/module_hsolver/hsolver_pw.cpp

+        DiagoDavid<T, Device> david(pre_op.get(),
                                    nband,
                                    dim,
                                    PARAM.inp.pw_diag_ndim,


besides the PreOP, now I realize the DiagoDavid class has the similar problem. It is confusing to instantiate the class DiagoDavid then name as david. A factory function will be better and more consistent, but will not be work of this PR:

std::unique_ptr<Diago> dav = make_diago("dav", /* other args */); // or use object directly without pointer

extract precondition function

a8bb37f

maki49 force-pushed the precondition branch from e24c141 to f146204 Compare November 14, 2024 19:47

apply to dav

4f4f356

maki49 force-pushed the precondition branch from f146204 to 4f4f356 Compare November 14, 2024 19:54

maki49 changed the title ~~Refactor: extract preconditioner as functions~~ Refactor: Preconditioner function library Nov 14, 2024

maki49 requested review from Cstandardlib, a1henu, denghuilu and haozhihan and removed request for Cstandardlib November 14, 2024 20:20

maki49 force-pushed the precondition branch 4 times, most recently from d88022c to 4cc41f5 Compare November 14, 2024 21:00

fix UTs

17cac27

maki49 force-pushed the precondition branch from 4cc41f5 to 17cac27 Compare November 14, 2024 21:29

kirk0830 requested a review from jieli-matrix November 15, 2024 01:35

Cstandardlib reviewed Nov 15, 2024

View reviewed changes

maki49 added 3 commits November 15, 2024 15:15

separate sorce and destination pointer

0687643

fix sdft test compile bug on icpx

3ac0de2

fix error passing host pointer to gpu kernel

a0571fb

kirk0830 removed the request for review from jieli-matrix November 15, 2024 10:53

mohanchen added the Feature Discussed The features will be discussed first but will not be implemented soon label Nov 17, 2024

mohanchen reviewed Nov 17, 2024

View reviewed changes

mohanchen closed this Nov 17, 2024

maki49 reopened this Nov 17, 2024

mohanchen closed this Nov 17, 2024

kirk0830 reviewed Nov 18, 2024

View reviewed changes

maki49 mentioned this pull request Nov 19, 2024

Question: integrated test case 140_PW_15_SO fails in PR#5517 and #5519 #5522

Closed

10 tasks

Refactor: Preconditioner function library #5482

Refactor: Preconditioner function library #5482

Uh oh!

Conversation

maki49 commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cstandardlib Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maki49 Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirk0830 Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maki49 Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohanchen commented Nov 17, 2024

Uh oh!

kirk0830 commented Nov 17, 2024

Uh oh!

kirk0830 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maki49 commented Nov 14, 2024 •

edited

Loading

Cstandardlib Nov 15, 2024 •

edited

Loading

maki49 Nov 15, 2024 •

edited

Loading

kirk0830 Nov 17, 2024 •

edited

Loading

maki49 Nov 17, 2024 •

edited

Loading