Skip to content

LCAO refactor step 4#3921

Merged
mohanchen merged 96 commits into
deepmodeling:developfrom
mohanchen:develop
Apr 8, 2024
Merged

LCAO refactor step 4#3921
mohanchen merged 96 commits into
deepmodeling:developfrom
mohanchen:develop

Conversation

@mohanchen

Copy link
Copy Markdown
Collaborator

The LCAO_hamilt.cpp was not adapted to current framework of the LCAO line. Most of the functions in the file are now only used for generating H-related matrices with the sparse format. Therefore, a series of spar_.cpp and spar_.h files are desinged instead in the LCAO code directory.

Linked Issue

Fix #3920
Fix #3866

@mohanchen mohanchen requested review from jinzx10 and kirk0830 April 7, 2024 00:53

@kirk0830 kirk0830 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this PR I have seen there are several main problems blocking further LCAO development:

  • inconsistent abbreviation
  • inconsistent uppercase and lowercase
  • too much inflated parameter table of too nested functions
  • ill-defined class
  • macro inserted parameter list

I really do not recommend to use using namespace std;

@jinzx10 jinzx10 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just got one small issue regarding the template function "cal_HR_exx" .

In module_hamilt_lcao/hamilt_lcaodft, this template function was declared in spar_exx.h, implemented in LCAO_hamilt.hpp, and used in spar_hsr.cpp. As such, I think this function is going to be instantiated & compiled during the compilation of spar_hsr.cpp. I notice that there's an independent source file "spar_exx.cpp", which include the template declaration & definition but does not explicitly instantiate the function. In this case, I think this cpp file does not work as it was supposed to do: its compiled object (spar_exx.o) would not contains the function "cal_HR_exx".

The above can be verified by by investigating the symbols of the object files. As shown below, spar_exx.o does not contain any symbol about cal_HR_exx, while spar_hsr.o does.
image

This little "mismatch" does not break anything because eventually the function is instantiated and compiled. Yet I think, for the sake of clarity, it might be better to either explicitly instantiate cal_HR_exx in spar_exx.cpp or just remove it.

@mohanchen

Copy link
Copy Markdown
Collaborator Author

Thanks for the comments, further refactoring by more developers is indeed needed.

@mohanchen mohanchen merged commit 4356f9f into deepmodeling:develop Apr 8, 2024
@mohanchen mohanchen added the The Absolute Zero Reduce the "entropy" of the code to 0 label May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

The Absolute Zero Reduce the "entropy" of the code to 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LCAO refactor Step 4:Eliminate the LCAO_hamilt.cpp file and related class Useless code should be deleted in LCAO

4 participants