Skip to content

module_dftu: pass nspin and onsite_radius explicitly, removing the two test access hacks it caused - #7940

Merged
mohanchen merged 2 commits into
deepmodeling:developfrom
Critsium-xy:refactor/dftu-explicit-input-args
Sep 9, 2026
Merged

mohanchen merged 2 commits into
deepmodeling:developfrom
Critsium-xy:refactor/dftu-explicit-input-args

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

Follows the method established by #7921, applied to the one module outside
source_io where it actually removes the macro.

Why these two files

#define private public has two independent root causes, and only one of them
is about PARAM:

  • (a) the code under test reads global PARAM itself, so the test can only
    drive it by writing PARAM.input / PARAM.sys, which are private;
  • (b) the test reaches into private members of the class under test.

Passing INPUT values explicitly fixes (a) and nothing else. I classified all 69
files that carry the macro by which header the macro region actually covers:
11 are pure-(a), 22 are (a)+(b) where the macro survives either way, and 36
are pure-(b). Outside source_io, module_dftu holds the only two pure-(a)
files in the tree — so this is the complete (a)-driven macro removal available
outside that module.

What changes

dftu_nao_op.cpp read two INPUT values out of the global singleton:
PARAM.inp.nspin in the constructor and PARAM.inp.onsite_radius inside
initialize_HR(). That is why dftu_lcao_test.cpp had to write PARAM.input,
and why it carried the macro.

Both now arrive as arguments. initialize_HR() is private and called only from
the constructor, so it simply takes onsite_radius; no new member was added for
it. dftu_nao_op.cpp now has zero PARAM references and no longer includes
parameter.h.

All five call sites are updated explicitly — no default arguments were added:

  • hamilt_lcao.cpp, gamma-only and multi-k branches;
  • force_stress_lcao.cpp, the tmpu force/stress instance;
  • the two cases in dftu_lcao_test.cpp, which pass values they already set locally.

Those first three are the composition roots for LCAO operators — the same role
relax_nsync.cpp plays in #7921. The global reads concentrate there and the leaf
operator stays clean.

dftu_pw_test.cpp needed no production change at all. It was parking its loop
variable in the singleton and reading it straight back — writing the private
half and reading the public one:

PARAM.input.nspin = c.nspin;
switch (PARAM.inp.nspin) { ... }
const double diag_coeff = PARAM.inp.nspin == 4 ? 1.0 : 0.5;

Nothing else reads it: the test's only production dependency,
source_pw/module_pwdft/dftu_base_tools.cpp, contains zero PARAM references.
c.nspin was already in scope.

Why both files are in one PR

The governance budget counts PARAM/GlobalV/GlobalC occurrences PR-wide and
blocks on added - removed > 0. Unlike #7921 — where relax_nsync.cpp already
held an injected Input_para* and the refactor was net −64 — both production
call sites here sit inside PARAM-dense functions with no local nspin in
scope, so moving the read out of the leaf costs budget at the call sites:

Δ
dftu_nao_op.cpp — the 2 reads removed −2
dftu_lcao_test.cpp — 3 PARAM.input writes removed −3
dftu_pw_test.cpp — 3 PARAM lines removed −3
3 production call sites — nspin + onsite_radius added +6
net_delta −2

Measured: added=6, removed=8, net_delta=-2. Split into two PRs, the second one
is +1 and CI blocks it. The dftu_pw_test cleanup is what pays for the refactor.

This is worth stating as a general point for the programme: (a)-type refactors
are budget-positive by construction
— one read point becomes N call sites — so
they need either an existing injection point or a paired cleanup in the same PR.

Test expectations are unchanged

No expected value was relaxed and no result.ref was regenerated.

Both dftu_lcao_test cases already set nspin explicitly (1 and 2) and
onsite_radius in SetUp (1.0), so nothing was relying on the Input_para
defaults — the regression that cost #7921 five suites when a new struct's
defaults did not mirror input_parameter.h. I checked every case before
touching them. The fixture member onsite_radius_test = 1.0 carries the value
SetUp used to write into PARAM.

Result

before after
files with the macro 69 67
occurrences 97 95
PARAM/GlobalV refs in module_dftu production code 9 7

The 7 remaining are ks_solver in dftu_nao_fs_k.cpp / dftu_nao_occ.cpp and
nspin in dftu_nao_energy.cpp. None is in either test's depend_files, so
touching them buys no macro and only costs budget.

Nothing here adds an #undef private or #undef protected, and no file whose
macro survives is touched — #7921's scope rule: remove the macro, or leave the
file alone.

Verification

Linux, cmake -B build -G Ninja -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ON:

  • build: 3318/3318 targets, 0 errors. Dropping the parameter.h include from
    dftu_nao_op.cpp exposed no transitive dependency.
  • ctest -R dftu: 5/5 pass, including both changed tests.
  • full unit suite: 29 of 338 fail — the failure set is byte-identical to the
    base commit
    (402aa8dab) built and run in a parallel worktree in the same
    environment. Both comm directions are empty: no new failures, no tests newly
    passing. The 29 are pre-existing and environment-related (mpirun-based
    *_para/*_parallel wrappers, plus cubic_spline, math_sphbes, dav,
    bpcg, PSI_init, ...).
  • agent_governance_check.py --base origin/develop --head HEAD: 0 errors,
    exit 0. The access-hack ratchet reports nothing at all (2 removed, 0 added).
  • Verified via compiled objects that all five changed files were actually built,
    rather than skipped by the local feature configuration.

Governance warnings and why: the 6 "Global dependency budget" hits are the
PARAM.inp.nspin / PARAM.inp.onsite_radius arguments added at the three call
sites, explained above; net delta is −2. The "Documentation sync review" warning
applies because no INPUT parameter behaviour changed — this is an internal
signature change only, so docs/parameters.yaml and
docs/advanced/input_files/input-main.md need no update.

What is deliberately not here

Reason-(b) files need a per-class decision in a production header — a public
const observer, an explicit friend, or turning a void method that writes a
private flag into one that returns it. That is the next tranche and is
independent of PARAM.

Reason-(a) elsewhere outside source_io no longer removes any macro: the six
source_md tests are driven entirely by five PARAM writes in setcell.h and
their integrators are already PARAM-free, but all six macros are (b);
source_estate's remaining (a) is nspin/nbands/nlocal, which is the
de-globalization programme rather than one PR.

🤖 Generated with Claude Code

Critsium-xy and others added 2 commits September 9, 2026 14:36
…ARAM

The test parked its loop variable in the global Parameter singleton and read
it straight back:

    PARAM.input.nspin = c.nspin;
    switch (PARAM.inp.nspin) { ... }
    const double diag_coeff = PARAM.inp.nspin == 4 ? 1.0 : 0.5;

It wrote the private half (`PARAM.input`) and read the public one
(`PARAM.inp`), so PARAM was serving as a scratch local -- and writing the
private half is the whole reason this file carried `#define private public`.

Nothing else reads it: the test's only production dependency,
source_pw/module_pwdft/dftu_base_tools.cpp, contains zero PARAM references.
`c.nspin` is already in scope, so use it directly.

The macro and the parameter.h include it guarded are removed with it. No
production code changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng PARAM

The DFTU LCAO operator read two INPUT values straight out of the global
Parameter singleton -- `PARAM.inp.nspin` in the constructor and
`PARAM.inp.onsite_radius` in `initialize_HR()`. That is why dftu_lcao_test.cpp
had to write `PARAM.input`, which is private, which is why it carried
`#define private public`.

Thread them through instead. `initialize_HR()` is private and called only from
the constructor, so it simply takes `onsite_radius` as an argument; the
constructor takes both and assigns the member. `dftu_nao_op.cpp` now has zero
PARAM references and no longer includes parameter.h.

All five call sites are updated explicitly -- no default arguments were added:

- hamilt_lcao.cpp, gamma-only and multi-k branches;
- force_stress_lcao.cpp, the `tmpu` force/stress instance;
- the two cases in dftu_lcao_test.cpp, which now pass the values they already
  set locally.

These first three are the composition roots for LCAO operators, the same role
relax_nsync.cpp plays in deepmodeling#7921: the global reads concentrate there and the leaf
operator stays clean.

The test's expectations are unchanged. Both cases already set nspin explicitly
(1 and 2) and onsite_radius in SetUp (1.0), so nothing was relying on the
Input_para defaults -- the regression that cost deepmodeling#7921 five suites. The fixture
member `onsite_radius_test = 1.0` carries the value SetUp used to write into
PARAM.

`#define private public` in dftu_lcao_test.cpp is removed with the reason for
it. Nothing in this commit adds an `#undef private`, and no file whose macro
survives is touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Sep 9, 2026
@mohanchen
mohanchen merged commit 7cc7d50 into deepmodeling:develop Sep 9, 2026
17 checks passed
@Critsium-xy
Critsium-xy deleted the refactor/dftu-explicit-input-args branch September 14, 2026 05:25
Critsium-xy added a commit to Critsium-xy/abacus-develop that referenced this pull request Sep 14, 2026
Continues the cleanup (deepmodeling#7940, deepmodeling#7949, deepmodeling#7952, deepmodeling#7953). Four macros come off, and
in three of the four files nothing is granted to anyone.

- `read_sep_test.cpp` is vestigial: it carries the macro but touches nothing
  private in `sep.h`. The directives are deleted and nothing else changes, as
  with `cal_test.cpp` and `test_hsolver.cpp` in deepmodeling#7921.

- `soc_test.cpp` read `soc.p_rot[l2p1*i + n]` five times, while `Soc` has a
  public `rotylm(i1, i2)` returning exactly `p_rot[l2plus1_*i1 + i2]`. Those
  become `soc.rotylm(i, n)` / `soc.rotylm(i+1, n)`.

  The sixth use was `EXPECT_NE(soc.p_rot, nullptr)`. That assertion is dropped
  rather than kept alive with a friend declaration: the next line already calls
  `soc.rotylm(0, 0)` and checks its value, which covers both "was it allocated"
  and "is it correct", and the guard was `EXPECT_` rather than `ASSERT_`, so it
  did not even stop the dereference that follows. `soc.h` is therefore
  untouched by this PR.

- `atom_spec_test.cpp` calls the private `Pseudopot_upf::read_pseudo_upf201`,
  which writes thirteen members of its object and has no stateless form. It
  gets `friend class AtomSpecTest;` alongside the `AtomPseudoTest`, `NCPPTest`
  and `ReadPPTest` grants already there from deepmodeling#7949, plus a forwarding wrapper.
  The `atom.type` and `atom.ncpp` accesses in the same file are public members
  of `Atom` and never needed the macro.

- `sltk_grid_test.cpp` wrote `PARAM.input.test_grid = 1` only to pass it into
  `Grid LatGrid(PARAM.input.test_grid)` -- PARAM used as a local, so it becomes
  one. Its genuine private access is `Grid::setMemberVariables`, 117 lines
  setting members from a UnitCell, so that gets `friend class SltkGridTest;`
  and a wrapper. `Grid::pbc` and `Grid::sradius2`, also read here, are public.

Friendship is not inherited and a TEST_F body lives in a derived class, hence
the wrappers -- the arrangement established in deepmodeling#7949.

An earlier revision also removed the macro from `memory_test.cpp` and the three
`propagator_test*.cpp`. Compiling without it showed both readings wrong:
`memory_test` reads `Memory::name`, `class_name`, `consume` and `init_flag`,
private statics reached through `::` rather than a member access; the propagator
tests read `PARAM.input`, so they are reason-(a) work. Both are left alone, per
the rule this series follows: remove the macro, or leave the file untouched.

Macro occurrences 88 -> 84. Production change is two `friend` declarations, one
of them added to a list that already exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohanchen pushed a commit that referenced this pull request Sep 14, 2026
Continues the cleanup (#7940, #7949, #7952, #7953). Four macros come off, and
in three of the four files nothing is granted to anyone.

- `read_sep_test.cpp` is vestigial: it carries the macro but touches nothing
  private in `sep.h`. The directives are deleted and nothing else changes, as
  with `cal_test.cpp` and `test_hsolver.cpp` in #7921.

- `soc_test.cpp` read `soc.p_rot[l2p1*i + n]` five times, while `Soc` has a
  public `rotylm(i1, i2)` returning exactly `p_rot[l2plus1_*i1 + i2]`. Those
  become `soc.rotylm(i, n)` / `soc.rotylm(i+1, n)`.

  The sixth use was `EXPECT_NE(soc.p_rot, nullptr)`. That assertion is dropped
  rather than kept alive with a friend declaration: the next line already calls
  `soc.rotylm(0, 0)` and checks its value, which covers both "was it allocated"
  and "is it correct", and the guard was `EXPECT_` rather than `ASSERT_`, so it
  did not even stop the dereference that follows. `soc.h` is therefore
  untouched by this PR.

- `atom_spec_test.cpp` calls the private `Pseudopot_upf::read_pseudo_upf201`,
  which writes thirteen members of its object and has no stateless form. It
  gets `friend class AtomSpecTest;` alongside the `AtomPseudoTest`, `NCPPTest`
  and `ReadPPTest` grants already there from #7949, plus a forwarding wrapper.
  The `atom.type` and `atom.ncpp` accesses in the same file are public members
  of `Atom` and never needed the macro.

- `sltk_grid_test.cpp` wrote `PARAM.input.test_grid = 1` only to pass it into
  `Grid LatGrid(PARAM.input.test_grid)` -- PARAM used as a local, so it becomes
  one. Its genuine private access is `Grid::setMemberVariables`, 117 lines
  setting members from a UnitCell, so that gets `friend class SltkGridTest;`
  and a wrapper. `Grid::pbc` and `Grid::sradius2`, also read here, are public.

Friendship is not inherited and a TEST_F body lives in a derived class, hence
the wrappers -- the arrangement established in #7949.

An earlier revision also removed the macro from `memory_test.cpp` and the three
`propagator_test*.cpp`. Compiling without it showed both readings wrong:
`memory_test` reads `Memory::name`, `class_name`, `consume` and `init_flag`,
private statics reached through `::` rather than a member access; the propagator
tests read `PARAM.input`, so they are reason-(a) work. Both are left alone, per
the rule this series follows: remove the macro, or leave the file untouched.

Macro occurrences 88 -> 84. Production change is two `friend` declarations, one
of them added to a list that already exists.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohanchen pushed a commit that referenced this pull request Sep 14, 2026
…public (#7966)

Continues the cleanup (#7940, #7949, #7952, #7953, and open #7963/#7964/#7965).
Two class families this time, handled by the same triage: use the accessor that
exists, and grant friendship only for what genuinely has no public route.

Measured against the TEST_F bodies -- accesses inside fixture members need
nothing, since the fixture is the friend -- the four files touch far less than
their size suggests: 10, 6, 5 and 1 sites respectively.

- **`pw_basis_k_test.cpp` needs nothing granted.** Its only non-public reads are
  `device` and `precision`, and `PW_Basis` already has `get_device()` /
  `get_precision()`.

- **`pw_basis_test.cpp`** reads the same two through those accessors, and calls
  three protected routines -- `distribute_g()`, `distribute_r()` and
  `getstartgr()` -- which set 7, 28 and 33 members of their object and have no
  stateless form. Those get `friend class ::PWBasisTEST;` and three forwarders.

- **`test_hsolver_pw.cpp`** has exactly one live call into protected territory,
  `hamiltSolvePsiK` (30 `this->`), in the NpwxLessThanNbandsDeath test; the
  other references to it and to `update_precondition` are commented out. It gets
  `friend class ::TestHSolverPW;` and one forwarder.

- **`test_hsolver_sdft.cpp`** is vestigial: every `TEST_F` in it is commented
  out, and the `nbands` it appeared to touch is `stowf.nbands_diag`, a member of
  a different class. The directives are simply deleted, as with `cal_test.cpp`
  and `test_hsolver.cpp` in #7921.

`FFT_Bundle` gains `get_device()` and `get_precision()`. The pw tests check that
`PW_Basis`'s constructor propagates device and precision into its `fft_bundle`,
which is a real behavioural check and not redundant, but `FFT_Bundle`'s copies
were private with no accessor. `PW_Basis` already exposes exactly this pair, so
this completes a parallel that was half-present rather than inventing an
accessor for a test.

Both friended classes live in a namespace (`ModulePW`, `hsolver`) while the
fixtures are at global scope, so each needs a global forward declaration and
`friend class ::Fixture;` -- an unqualified `friend class Fixture;` would name a
nonexistent class inside the namespace and silently grant nothing.

Occurrences tree-wide 88 -> 84. Production change is three friend declarations
with their forward declarations, plus the two `FFT_Bundle` accessors.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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