Skip to content

Bug: present Numerical_Basis memory management is risky #3605

@kirk0830

Description

@kirk0830

Describe the Code Quality Issue

Present support of multiple bessel_nao_rcut values may trigger a bug that output orb_matrix* files are broken but unawared by ABACUS. It is because

        if (winput::out_spillage <= 2)
        {
            Numerical_Basis numerical_basis;
            if(INPUT.bessel_nao_rcuts.size() == 1)
            {
                numerical_basis.output_overlap(this->psi[0], this->sf, this->kv, this->pw_wfc);
            }
            else
            {
                for(int i = 0; i < INPUT.bessel_nao_rcuts.size(); i++)
                {
                    if(GlobalV::MY_RANK == 0) {std::cout << "update value: bessel_nao_rcut <- " << std::fixed << INPUT.bessel_nao_rcuts[i] << " a.u." << std::endl;}
                    INPUT.bessel_nao_rcut = INPUT.bessel_nao_rcuts[i];
                    numerical_basis.output_overlap(this->psi[0], this->sf, this->kv, this->pw_wfc);
                    std::string old_fname_header = winput::spillage_outdir + "/" + "orb_matrix.";
                    std::string new_fname_header = winput::spillage_outdir + "/" + "orb_matrix_rcut" + std::to_string(int(INPUT.bessel_nao_rcut)) + "deriv";
                    for(int derivative_order = 0; derivative_order <= 1; derivative_order++)
                    {
                        // rename generated files
                        std::string old_fname = old_fname_header + std::to_string(derivative_order) + ".dat";
                        std::string new_fname = new_fname_header + std::to_string(derivative_order) + ".dat";
                        std::rename(old_fname.c_str(), new_fname.c_str());
                    }
                }
            }
            ModuleBase::GlobalFunc::DONE(GlobalV::ofs_running, "BASIS OVERLAP (Q and S) GENERATION.");
        }

if change to

        if (winput::out_spillage <= 2)
        {
            if(INPUT.bessel_nao_rcuts.size() == 1)
            {
                Numerical_Basis numerical_basis;
                numerical_basis.output_overlap(this->psi[0], this->sf, this->kv, this->pw_wfc);
            }
            else
            {
                for(int i = 0; i < INPUT.bessel_nao_rcuts.size(); i++)
                {
                    if(GlobalV::MY_RANK == 0) {std::cout << "update value: bessel_nao_rcut <- " << std::fixed << INPUT.bessel_nao_rcuts[i] << " a.u." << std::endl;}
                    INPUT.bessel_nao_rcut = INPUT.bessel_nao_rcuts[i];
                    Numerical_Basis numerical_basis;
                    numerical_basis.output_overlap(this->psi[0], this->sf, this->kv, this->pw_wfc);
                    std::string old_fname_header = winput::spillage_outdir + "/" + "orb_matrix.";
                    // well actually the bessel_nao_rcut is not necessary to be integer, present way is because of historically reason.
                    std::string new_fname_header = winput::spillage_outdir + "/" + "orb_matrix_rcut" + std::to_string(int(INPUT.bessel_nao_rcut)) + "deriv";
                    for(int derivative_order = 0; derivative_order <= 1; derivative_order++)
                    {
                        // rename generated files
                        std::string old_fname = old_fname_header + std::to_string(derivative_order) + ".dat";
                        std::string new_fname = new_fname_header + std::to_string(derivative_order) + ".dat";
                        std::rename(old_fname.c_str(), new_fname.c_str());
                    }
                }
            }
            ModuleBase::GlobalFunc::DONE(GlobalV::ofs_running, "BASIS OVERLAP (Q and S) GENERATION.");
        }

then orb_matrix* can have complete information. But it implies the memory management inside Numerical_Basis is not-well-designed, seems it highly relies on auto-destroy instead of being with a mechanism to refresh/reset its data if want a second use, which means, therefore the management of data and calculation of data are within the same class, and this class is not designed clearly in aspect whether it SHOULD BE a singleton or common object for its instance.

During refactor of orbital generation suites, I also find there may be only one parameter in INPUTw is needed, say the out_spillage, although it seems the orbital generation code can also be used to "wannierize" selected planewave wavefunction, but the INPUTw related functionalities are in state of nobody-maintained.
Have plan to merge settings in INPUTw to INPUT.

Additional Context

No response

Task list for Issue attackers (only for developers)

  • Identify the specific code file or section with the code quality issue.
  • Investigate the issue and determine the root cause.
  • Research best practices and potential solutions for the identified issue.
  • Refactor the code to improve code quality, following the suggested solution.
  • Ensure the refactored code adheres to the project's coding standards.
  • Test the refactored code to ensure it functions as expected.
  • Update any relevant documentation, if necessary.
  • Submit a pull request with the refactored code and a description of the changes made.

Metadata

Metadata

Assignees

Labels

BugsBugs that only solvable with sufficient knowledge of DFT

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions