Skip to content

source_basis/module_ao: use the getters that already exist instead of #define private public - #7952

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/orb-tests-use-getters
Sep 12, 2026
Merged

source_basis/module_ao: use the getters that already exist instead of #define private public#7952
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:refactor/orb-tests-use-getters

Conversation

@Critsium-xy

@Critsium-xy Critsium-xy commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Third tranche of the #define private public cleanup, after #7940 (tests
driving global PARAM) and #7949 (tests calling private methods). This one
covers the case where the member the test reaches for already has a public
getter — so the macro comes off with no production change at all.

Three cases, and the middle one is a trap

orb_nonlocal_test.cpp and orb_read_test.cpp reach into Numerical_Nonlocal
and LCAO_Orbitals. Neither file touches PARAM; nothing is added to a
production header. The entire diff is in the two test files.

1. The member is used as a plain value — read it through its getter.
lcao_.kmesh becomes lcao_.get_kmesh() (4 sites); the five SetTypeInfo
assertions become nn.getLabel(), getType(), getLmax(), get_rcut_max(),
get_nproj(). Those already compared against the fixture's inputs, so they stay
real checks.

2. The assertion is EXPECT_EQ(obj.get_x(), obj.x). Substituting the
getter here would produce EXPECT_EQ(get_x(), get_x()) — an assertion that
cannot fail. Mechanically removing the macro this way would leave the test
green and empty, which is worse than the macro. These are re-anchored to the
value the object was actually given.

orb_nonlocal_test already had one line in the right form
(EXPECT_EQ(nn.get_rcut_max(), rcut_max_)); the other four now match it.

In LCAO_Orbitals::Getters, seven of the twelve assertions covered private
members. ntype and lmax are passed straight into Read_Orbitals, so they
anchor to ntype_ / lmax_. The remaining four are derived, and the
production formula is deliberately not restated in the test — a test that
recomputes the thing it is checking passes even when the formula is wrong. They
are asserted as the concrete values this fixture implies, each with its
provenance in a comment:

value where it comes from
kmesh 1113 int(sqrt(ecutwfc)/dk) + 4 = int(sqrt(123)/0.01) + 4
nchimax 2 H is 2s1p, O is 2s2p1d
lmax_d, nchimax_d 2 from jle.orb
rcutmax_Phi 8 au H is 8 au, O is 7 au

Each was measured against the built test, and kmesh additionally cross-checked
by hand against Read_Orbitals so the assertion records the correct value rather
than merely the observed one.

3. The test wrote a private member to build a fixture —
nnl[i].rcut = 1.0 in NumericalNonlocalTest::SetUp.
Numerical_Nonlocal_Lm derives rcut from the last point of its radial mesh
(rcut = r_radial_in[nr-1]), so each projector is now built through the public
set_NL_proj() with a minimal three-point mesh whose endpoint is the wanted
rcut. This removes the write and exercises set_NL_proj, which no assertion in
this file reached before.

ecutwfc, dk, dR, Rmax and dr_uniform are public members of
LCAO_Orbitals and never needed the macro. The four that have a corresponding
fixture input are anchored to it for consistency; dr_uniform is left as it was.

Result

Macro occurrences in these two files: 2 -> 0. No #undef private is added,
and no file whose macro survives is touched.

Verification

Linux, cmake -B build -G Ninja -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ON,
followed by cmake --install build — which matters here: the module_ao tests
get their orbital data from install(DIRECTORY lcao_H2O ...), so without the
install step ORB_read_test fails and ORB_atomic_lm_test /
ORB_nonlocal_lm_test segfault on missing input, both before and after this
change.

  • build: 0 errors (branch and base both BUILD_EXIT=0).
  • MODULE_AO_ORB_nonlocal_test, MODULE_AO_ORB_read_test,
    MODULE_AO_ORB_atomic_lm_test, MODULE_AO_ORB_nonlocal_lm_test: all pass.
  • full unit suite: 2 of 339 fail — the failure set is identical to the base
    commit     (9cbcc0b55) built, installed and run in a parallel worktree in the
    same environment; both comm directions are empty. The two are
    MODULE_HSOLVER_diago_hs_parallel and MODULE_HSOLVER_LCAO, both
    mpirun-based and pre-existing.
  • agent_governance_check.py: 0 errors. The access-hack ratchet reports
    nothing (2 removed, 0 added) and no PARAM/GlobalV/GlobalC reference is
    added or removed, so the global-dependency budget is untouched.

No INPUT parameter and no user-visible behaviour changed, so
docs/parameters.yaml and docs/advanced/input_files/input-main.md need no
update.

What is not here

The other two module_ao tests, orb_atomic_lm_test.cpp (887 lines) and
orb_nonlocal_lm_test.cpp (755 lines), hold the bulk of this category — roughly
190 further sites where a member with an existing getter is read directly. They
are left for a separate PR because they cannot be finished the same way: each
also calls private methods (cal_kradial, cal_rradial_sbpool, plot,
freemem, renew, get_kradial) and orb_atomic_lm_test reads psir, which
has no getter. Those need a friend grant in a production header — the #7949
pattern — which is a different review question from "use the accessor that
already exists", and mixing the two would obscure both.

Their getter-vs-member block has the same shape as the one fixed here but over
arrays (get_psi() vs psi), where there is no scalar input to re-anchor to;
that block is better routed through the fixture once the friend is in place.

🤖 Generated with Claude Code

@Critsium-xy @claude
source_basis/module_ao: drop the access hack from two orbital tests, …
21cb136 
…no production change

`orb_nonlocal_test.cpp` and `orb_read_test.cpp` switched off access control for
their whole translation unit to reach members of `Numerical_Nonlocal` and
`LCAO_Orbitals` that mostly already have public getters. Neither file touches
PARAM, and neither needs anything added to a production header: the macro is
removed by using the accessors that exist, plus one fixture built through the
public API instead of by assignment.

Three distinct cases, and the second is the interesting one:

1. The member is used as a plain value. Read it through its getter --
   `lcao_.kmesh` -> `lcao_.get_kmesh()` (4 sites), and the five assertions in
   `SetTypeInfo` now read `nn.getLabel()`, `getType()`, `getLmax()`,
   `get_rcut_max()`, `get_nproj()`. Those already compared against the fixture's
   inputs, so they stay real checks.

2. The assertion *is* `EXPECT_EQ(obj.get_x(), obj.x)`. Substituting the getter
   would turn it into `EXPECT_EQ(get_x(), get_x())` -- it cannot fail, and the
   test would be silently gutted. These are re-anchored to the value the object
   was given instead, which is what `orb_nonlocal_test` already did on one line
   (`EXPECT_EQ(nn.get_rcut_max(), rcut_max_)`); the rest now match it.

   In `LCAO_Orbitals::Getters`, seven of the twelve assertions covered private
   members. `ntype` and `lmax` are passed straight into `Read_Orbitals`, so they
   anchor to `ntype_` / `lmax_`. The other four are derived, and the production
   formula is deliberately *not* restated in the test -- a test that recomputes
   what it is checking passes even when the formula is wrong. They are asserted
   as the concrete values this fixture implies, each with its provenance:
   `kmesh` 1113 = int(sqrt(123)/0.01) + 4, `nchimax` 2 (H is 2s1p, O is 2s2p1d),
   `lmax_d`/`nchimax_d` 2 from jle.orb, `rcutmax_Phi` 8 au (H 8 au, O 7 au).
   Measured against the built test to confirm, and kmesh cross-checked by hand
   against Read_Orbitals.

3. The test *wrote* a private member to build a fixture:
   `nnl[i].rcut = 1.0` in `NumericalNonlocalTest::SetUp`. `Numerical_Nonlocal_Lm`
   derives rcut from the last point of its radial mesh, so each projector is now
   built through the public `set_NL_proj()` with a minimal three-point mesh whose
   endpoint is the wanted rcut. That removes the write and additionally exercises
   `set_NL_proj`, which no assertion in this file reached before.

`ecutwfc`, `dk`, `dR`, `Rmax` and `dr_uniform` are public members of
`LCAO_Orbitals` and never needed the macro; the four that compare against a
fixture input are anchored to it for consistency, and `dr_uniform` is left alone.

Macro occurrences in these two files go 2 -> 0. No `#undef private` is added, and
no file whose macro survives is touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Critsium-xy Critsium-xy mentioned this pull request Sep 11, 2026
Merged
mohanchen
mohanchen approved these changes Sep 12, 2026
View reviewed changes

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

LGTM

@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Sep 12, 2026
@mohanchen
mohanchen merged commit 9958006 into deepmodeling:develop Sep 12, 2026
17 checks passed
@Critsium-xy
Critsium-xy deleted the refactor/orb-tests-use-getters branch September 14, 2026 05:25
@Critsium-xy Critsium-xy mentioned this pull request Sep 14, 2026
Merged
Critsium-xy added a commit to Critsium-xy/abacus-develop that referenced this pull request Sep 14, 2026
@Critsium-xy @claude
source_cell / source_pw: remove four more #define private public
7cc03aa 
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>
@Critsium-xy Critsium-xy mentioned this pull request Sep 14, 2026
Merged
mohanchen pushed a commit that referenced this pull request Sep 14, 2026
@Critsium-xy @claude
source_cell / source_pw: remove four more #define private public (#7963)
501e673 
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
@Critsium-xy @claude
source_basis/module_pw + source_hsolver: remove four #define private …
37c09ba 
…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>
This was referenced Sep 17, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mohanchen mohanchen mohanchen approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Critsium-xy @mohanchen