Skip to content

Refactor source_psi, remove some unnecessary dependency - #7675

Open
mohanchen wants to merge 16 commits into
deepmodeling:developfrom
mohanchen:2026-07-23-b
Open

Refactor source_psi, remove some unnecessary dependency#7675
mohanchen wants to merge 16 commits into
deepmodeling:developfrom
mohanchen:2026-07-23-b

Conversation

@mohanchen

Copy link
Copy Markdown
Collaborator

Refactor source_psi, remove some unnecessary dependency

@mohanchen
mohanchen requested a review from Critsium-xy July 23, 2026 05:39
@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Jul 23, 2026

@Critsium-xy Critsium-xy 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.

There are some not small problems🫠

#include <type_traits>
#include <vector>

using namespace std;

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.

using namespace std; in a header should be dropped before merge.

This header is included fairly widely -- psi_base.h -> psi_prepare.h -> setup_psi_pw.h -> ctrl_output_pw.h -- so this injects all of std into the global namespace across a large number of translation units.

The practical risk is ambiguous-overload and name-collision errors that are hard to trace back to their origin, since the failure surfaces in some unrelated file that merely includes this one transitively. Names like count, size, data, left, plus and complex are common enough in this codebase that a future free function or variable with a matching name can break the build in a way that gives no hint about where it came from.

It also is not needed here: every declaration in this file is already fully qualified (std::vector<int>, std::unique_ptr, std::string). Suggest simply deleting the line.

@Critsium-xy Critsium-xy 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.

Second comment: a design suggestion on the new prepare_params() / initialize() split. Not a correctness problem, but since it is an interface shape this PR newly introduces, it is much cheaper to settle now than after callers accumulate.

(This is dyzheng play😋)

Comment on lines +94 to +103
psi_init_atomic<T>* atomic_initer = new psi_init_atomic<T>();
atomic_initer->prepare_params(
PARAM.globalv.nqx,
PARAM.globalv.dq,
PARAM.inp.nspin,
PARAM.globalv.domag,
PARAM.globalv.domag_z,
PARAM.inp.pseudo_mesh
);
this->psi_initer = std::unique_ptr<psi_base<T>>(atomic_initer);

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.

Consider passing these parameters through the derived-class constructors instead of a separate prepare_params().

Every value handed to prepare_params() is already known at the point of construction, so the two-phase sequence buys nothing but adds a temporal coupling that has to be defended at runtime. The cost is spread over several places:

  1. A params_prepared_ flag plus a WARNING_QUIT guard in both psi_init_atomic::initialize (psi_init_atomic.cpp:69) and psi_init_nao::initialize (psi_init_nao.cpp:174), plus the separate nqx_ <= 0 guards in allocate_ps_table() and psi_init_nao::tabulate().
  2. The @see prepare_params() / "must be called before initialize()" note duplicated across five derived headers.
  3. Here, in five of the seven branches, the object has to be held in a derived-type raw pointer first and only then moved into the base-class unique_ptr, purely so prepare_params() can be reached.

With constructor parameters the branch collapses to the same shape as the random branch:

else if (this->init_wfc == "atomic"
         || (this->init_wfc == "atomic+random" && this->ucell.natomwfc < PARAM.inp.nbands))
{
    // ... existing ofs_running logging ...
    this->psi_initer = std::unique_ptr<psi_base<T>>(new psi_init_atomic<T>(
        PARAM.globalv.nqx, PARAM.globalv.dq, PARAM.inp.nspin,
        PARAM.globalv.domag, PARAM.globalv.domag_z, PARAM.inp.pseudo_mesh));
}

That lets you delete both params_prepared_ members, all four runtime guards, and the downcast in all five branches. More importantly, forgetting to supply the parameters stops being a runtime abort and becomes a compile error, which is the real reason to prefer it.

A related point: the current split between the two functions is not principled. nspin goes through prepare_params(), while npol and nbands -- equally pure configuration -- go through initialize(). Constructor for configuration, initialize() for linking the external objects (sf, pw_wfc, p_ucell, k-point data) draws that line in a way a reader can predict.

One caveat: psi_init_nao is also constructed in to_wannier90_lcao_in_pw.cpp:47, so that call site would need the same one-line change.

@Critsium-xy Critsium-xy 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.

Three minor cleanup points, none blocking.

#define PSI_INIT_RANDOM_H

#include <vector>
#include "source_pw/module_pwdft/vnl_pw.h"

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.

Leftover includes -- the PR's own dependency-removal goal is not quite finished:

  • vnl_pw.h is still here, though this is the one derived header that kept it; the other four dropped it in this PR.
  • parameter.h still included in psi_base.cpp:16, psi_init_atomic_random.cpp:3 and psi_init_nao_random.cpp:3, although none of those three files reference PARAM any more after this change.
  • The new #include "source_psi/psi_base.h" in setup_psi_pw.h is redundant: line 4 of that file already includes psi_prepare.h, which includes psi_base.h.
  • The three forward declarations added to psi_prepare.h (UnitCell, Structure_Factor, ModulePW::PW_Basis_K) are no-ops, since the psi_base.h it includes already provides all three full definitions.

Comment on lines +118 to +120
Atom* atom = &this->p_ucell_->atoms[it];

GlobalV::ofs_running<<"\n number of pseudo atomic orbitals for "<<atom->label<<" is "<< atom->ncpp.nchi << std::endl;
GlobalV::ofs_running<<"\n number of pseudo atomic orbitals for "<<atom->label<<" is "<< atom->ncpp.nchi << std::endl;

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.

Indentation regression: these two lines went from tabs to a single tab, which now under-indents them relative to the surrounding 8-space loop body rather than fixing the alignment.

Same pattern elsewhere in the PR: the reformatted if/else blocks in setup_psi_pw.cpp introduce new \t{ and \telse lines, and the latvec assignment block in psi_init_test.cpp gains tab-indented continuation lines. Since the PR is reformatting these files anyway, the direction should be tabs to spaces, not the reverse.

~PSIPrepare(){};

///@brief prepare the wavefunction initialization
void prepare_init(const int& random_seed);

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.

The documentation changes point in two opposite directions. Here the doxygen on prepare_init, initialize_psi and initialize_lcao_in_pw is deleted, along with the ///< comments on the members below. Meanwhile the same 10-line @param block is copied verbatim into the initialize() override in all five derived headers.

Overrides generally do not need their own copy -- one description on the base declaration covers them. Suggest the reverse of what the PR does: keep the docs here, and drop the duplicated blocks (or reduce them to @copydoc). Note also that the copy in psi_init_atomic_random.h carries @see prepare_params() for a function that class only inherits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants