Refactor namespace Exx_Opt_Orb #5226

PeizeLin · 2024-10-13T14:58:48Z

No description provided.

kirk0830

this PR do some minor refactor on codes that add "this->" to instance named "orb_", which makes the code clearer, this is acceptable.
However, it also shows me very astonishing use of nested std::map (I had never known them before), and very long lambda function that makes me confused about the program design.
There are also parameters packed in struct and named as "info", which I think, will bring more difficulty on program maintainment, so I request a change on it.

The use of extern keyword seems confusing and needs more explanation.

There are also new code added which depends explicitly on instance of class LCAO_Orbitals (is it a to-be-deprecated class? @mohanchen ).

Because seems there are already many codes in this module, so an immediate thoroughful improvement on code structure is nearly impossible, I recommend not introduce more cognition complexity into codes.

kirk0830 · 2024-10-15T05:41:22Z

source/module_io/input_conv.cpp

-        Exx_Abfs::Jle::Lmax = PARAM.inp.exx_opt_orb_lmax;
-        Exx_Abfs::Jle::Ecut_exx = PARAM.inp.exx_opt_orb_ecut;
-        Exx_Abfs::Jle::tolerence = PARAM.inp.exx_opt_orb_tolerence;
+        GlobalC::exx_info.info_opt_abfs.abfs_Lmax = PARAM.inp.exx_opt_orb_lmax;


use of GlobalC is highly discouraged

kirk0830 · 2024-10-15T05:44:18Z

source/module_ri/exx_abfs-construct_orbs.h

 		const LCAO_Orbitals &orb_in,
 		const double kmesh_times );
-	static std::vector<std::vector<std::vector<Numerical_Orbital_Lm>>> change_orbs( 
+	extern std::vector<std::vector<std::vector<Numerical_Orbital_Lm>>> change_orbs(


could you explain why you change all static to extern???
from https://en.cppreference.com/w/cpp/keyword/extern, which is the result want to get?

kirk0830 · 2024-10-15T05:46:45Z

source/module_ri/exx_abfs-jle.cpp

-void Exx_Abfs::Jle::init_jle( const double kmesh_times, const LCAO_Orbitals& orb )
+std::vector< std::vector< std::vector< Numerical_Orbital_Lm>>>
+Exx_Abfs::Jle::init_jle(
+	const Exx_Info::Exx_Info_Opt_ABFs &info,


the "info" used here has very straight forward drawback: it is impossible to know what one function really needs according to its parameter list. This will brings difficulty to code maintaining, unless this code is not expected to be maintained by any other

kirk0830 · 2024-10-15T05:47:28Z

source/module_ri/exx_opt_orb-print.cpp

-	const ModuleBase::Element_Basis_Index::IndexLNM &index_jles) const
+	const std::vector<double>& orb_cutoff,
+	const ModuleBase::Element_Basis_Index::Range &range_jles,
+	const ModuleBase::Element_Basis_Index::IndexLNM &index_jles)
 {
 	auto print_header = [&]( std::ofstream &ofs )


why use lambda here?

kirk0830 · 2024-10-15T05:49:09Z

source/module_ri/exx_opt_orb-print.cpp

-	
-	
+
+
 	auto print_Q = [&]( std::ofstream &ofs )


why use lambda here?

PeizeLin · 2024-10-15T08:08:29Z

this PR do some minor refactor on codes that add "this->" to instance named "orb_", which makes the code clearer, this is acceptable. However, it also shows me very astonishing use of nested std::map (I had never known them before), and very long lambda function that makes me confused about the program design. There are also parameters packed in struct and named as "info", which I think, will bring more difficulty on program maintainment, so I request a change on it.

The use of extern keyword seems confusing and needs more explanation.

There are also new code added which depends explicitly on instance of class LCAO_Orbitals (is it a to-be-deprecated class? @mohanchen ).

Because seems there are already many codes in this module, so an immediate thoroughful improvement on code structure is nearly impossible, I recommend not introduce more cognition complexity into codes.

This PR is just to fix these codes from at least five years ago, making it can be run again under the LibRI framework. Many years ago, the codes of exx without LibRI were completely different from today.
So if you want to make more modifications to this code, I think it's better to be put in the future PR.

==============================

As for info, its definition can be seen in exx_info.h.
Perhaps the instantiation can be moved from GlobalC to PARAM in the future?

kirk0830 · 2024-10-15T09:02:57Z

@PeizeLin Hi, I understand the historical reason you mentioned.
There are reasons why I call "info" brings difficulty to code maintainment:

As the rule of thumb, functions should only take variables they really need, otherwise all possible caller will have an unnecessary long parameter list, or say it appears to be something like other developers are forced to know about exx_info instead of merely understand the function itself
a single call to function whose parameter list have an exx_info datatype obj, should be behind the construction of exx_info, or an explicit inclusion of file exx_info.h, this means a strong coupling between struct exx_info and functions. This will bring inconvienence on unittest which partially ensuring the function can work as expected.

PeizeLin · 2024-10-18T15:12:09Z

@PeizeLin Hi, I understand the historical reason you mentioned. There are reasons why I call "info" brings difficulty to code maintainment:

As the rule of thumb, functions should only take variables they really need, otherwise all possible caller will have an unnecessary long parameter list, or say it appears to be something like other developers are forced to know about exx_info instead of merely understand the function itself

a single call to function whose parameter list have an exx_info datatype obj, should be behind the construction of exx_info, or an explicit inclusion of file exx_info.h, this means a strong coupling between struct exx_info and functions. This will bring inconvienence on unittest which partially ensuring the function can work as expected.

All the variables in info is needed in the function. They come from INPUT.
I think it's better to use this four variables struct as the formal parameter explicitly than use the static variable PARAM.inp.xxx without formal parameter?
Maybe finally the actual argument is also the static variable PARAM.exx_info.info_opt_abfs when invoked, I think for this function itself, the formal parameter is better than static variable in the function at least.

kirk0830

I retract my file change request presently

mohanchen · 2024-11-14T13:47:22Z

I have seen new PR, so this one will be closed.

PeizeLin force-pushed the Exx_Opt_Orb branch from 31fcd02 to b130c6f Compare October 13, 2024 15:18

Refactor namespace Exx_Opt_Orb

ff65831

PeizeLin force-pushed the Exx_Opt_Orb branch from b130c6f to ff65831 Compare October 13, 2024 16:54

kirk0830 requested changes Oct 15, 2024

View reviewed changes

mohanchen added EXX and lr-TDDFT Related to EXX, this label is designed only for PeizeLin and Maki49 Refactor Refactor ABACUS codes labels Oct 17, 2024

PeizeLin added 5 commits October 25, 2024 02:58

update toolchain/install_libri.sh to v0.2.1.1

71ab505

update git submodule LibRI v0.2.1.1

2a12722

update Exx_Opt_Orb::print_matrix()

33539b5

Merge branch 'develop' into Exx_Opt_Orb

6bf64a9

Merge branch 'develop' into Exx_Opt_Orb

429ad10

kirk0830 reviewed Nov 7, 2024

View reviewed changes

Merge branch 'develop' into Exx_Opt_Orb

b53a473

mohanchen closed this Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor namespace Exx_Opt_Orb #5226

Refactor namespace Exx_Opt_Orb #5226

PeizeLin commented Oct 13, 2024

kirk0830 left a comment

kirk0830 Oct 15, 2024

kirk0830 Oct 15, 2024

kirk0830 Oct 15, 2024

kirk0830 Oct 15, 2024

PeizeLin Nov 6, 2024

kirk0830 Oct 15, 2024

PeizeLin Nov 6, 2024

PeizeLin commented Oct 15, 2024 •

edited

Loading

kirk0830 commented Oct 15, 2024 •

edited

Loading

PeizeLin commented Oct 18, 2024 •

edited

Loading

kirk0830 left a comment

mohanchen commented Nov 14, 2024

Refactor namespace Exx_Opt_Orb #5226

Refactor namespace Exx_Opt_Orb #5226

Conversation

PeizeLin commented Oct 13, 2024

kirk0830 left a comment

Choose a reason for hiding this comment

kirk0830 Oct 15, 2024

Choose a reason for hiding this comment

kirk0830 Oct 15, 2024

Choose a reason for hiding this comment

kirk0830 Oct 15, 2024

Choose a reason for hiding this comment

kirk0830 Oct 15, 2024

Choose a reason for hiding this comment

PeizeLin Nov 6, 2024

Choose a reason for hiding this comment

kirk0830 Oct 15, 2024

Choose a reason for hiding this comment

PeizeLin Nov 6, 2024

Choose a reason for hiding this comment

PeizeLin commented Oct 15, 2024 • edited Loading

kirk0830 commented Oct 15, 2024 • edited Loading

PeizeLin commented Oct 18, 2024 • edited Loading

kirk0830 left a comment

Choose a reason for hiding this comment

mohanchen commented Nov 14, 2024

PeizeLin commented Oct 15, 2024 •

edited

Loading

kirk0830 commented Oct 15, 2024 •

edited

Loading

PeizeLin commented Oct 18, 2024 •

edited

Loading