Skip to content

Refactor source_psi, remove some unnecessary dependency - #7675

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

Refactor source_psi, remove some unnecessary dependency#7675
mohanchen wants to merge 19 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🫠

Comment thread source/source_psi/psi_base.h

@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 thread source/source_psi/psi_prepare.cpp

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

Comment thread source/source_psi/psi_init_random.h Outdated
Comment thread source/source_psi/psi_init_atomic.cpp Outdated
Comment thread source/source_psi/psi_prepare.h Outdated
*/
template <typename T>
class psi_initializer
class psi_base

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.

Naming: psi_base is a weaker name than psi_initializer, and it collides conceptually with psi::Psi.

psi_initializer named the responsibility; psi_base names a position in the class hierarchy. The responsibility has not changed -- the comment right above this line still says "This class is used to allocate memory and give initial guess for psi", and the usage sequence documented there is still new->initialize->tabulate->init_psig. So the new name carries strictly less information than the old one.

The more concrete problem is that the file now sits next to source_psi/psi.h as source_psi/psi_base.h, which reads as "base class of psi::Psi". No such relationship exists, and a reader has to open the file to find that out.

If the goal is just to shorten the psi_initializer_* prefix -- and psi_init_atomic etc. are a genuine improvement -- then psi_init_base (or psi_init) keeps that gain while still describing the role, and matches the new CMake target name psi_init introduced in this PR. Worth settling before the name spreads: it is a sed today, and a much wider diff once other modules start referring to it.

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