deepmodeling / abacus-develop

An electronic structure package based on either plane wave basis or numerical atomic orbitals.
http://abacus.ustc.edu.cn
GNU Lesser General Public License v3.0
173 stars 132 forks source link

Refactoring & Optimization: Streamlining Density Matrix Management with respect to `Local Orbital Charge` #4090

Closed AsTonyshment closed 4 months ago

AsTonyshment commented 6 months ago

Describe the Code Quality Issue

Background

The Local Orbital Charge class, a significant component within our LCAO-related codebase, is presently marked by extensive global class usage. This widespread reliance on global classes has notably impaired the code's maintainability, making it challenging to integrate unit tests and new features without escalating the cost of maintenance.

What to do

Our primary objective is to begin disentangling the dependencies around the density matrix (DM) representation within the Local Orbital Charge class. This task serves as the initial step in a broader plan to enhance code modularity and reduce global dependencies.

The current approach involves direct manipulation of DM across various parts of the project, which has contributed to tightly coupled code and complex maintenance challenges. To address these issues, the immediate plan includes segregating functions directly dependent on DM into a dedicated file within the module_io directory. Subsequently, the old DM will be replaced progressively with the newly structured density matrix format (final goal).

This refactoring is not merely a reorganization of existing code but a strategic move towards a more modular and maintainable structure. By minimizing direct interactions with global variables and enhancing the clarity of data flows within our software, we aim to significantly improve the robustness and testability of our codebase.

Furthermore, functions like output_dm and output_dm1 that manage density matrix outputs will be scrutinized for their current roles and differences to ensure that their functionalities align with the new architectural standards.

Details regarding the phased implementation of these changes will follow. This initial issue aims to set the groundwork for subsequent updates that will further encapsulate and isolate functionality, paving the way for a cleaner, more efficient codebase.

Additional Context

No response

Task list for Issue attackers (only for developers)

WHUweiqingzhou commented 6 months ago

@AsTonyshment Actually, LOC class has been replaced by DensityMatrix class, and only use for out_dm and EXX calculations now. I think this class will be deleted soon. Why do you want to refactor it?

AsTonyshment commented 6 months ago

@AsTonyshment Actually, LOC class has been replaced by DensityMatrix class, and only use for out_dm and EXX calculations now. I think this class will be deleted soon. Why do you want to refactor it?

Yes, actually I don't mean to refactor LOC itself, but to refactor the codes using LOC. You get it right (as I've already written in the original description, see "final goal"), all codes using LOC will eventually be replaced by DensityMatrix class. I had a discussion with @mohanchen, and this issue is the first step, which intend to peel off the direct dependency on LOC to facilitate subsequent refactoring.