Skip to content

source_relax: pass INPUT values explicitly instead of reading PARAM, removing the test access hacks it caused - #7921

Open
Critsium-xy wants to merge 5 commits into
deepmodeling:developfrom
Critsium-xy:refactor/param-to-explicit-args
Open

source_relax: pass INPUT values explicitly instead of reading PARAM, removing the test access hacks it caused#7921
Critsium-xy wants to merge 5 commits into
deepmodeling:developfrom
Critsium-xy:refactor/param-to-explicit-args

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

Replaces #7919, which took a different route. That PR added
Parameter::input_for_test() so tests could write PARAM.input without
#define private public. It worked, but it removed the macro without removing
the reason the macro existed, and it grew a test-only door on a production
class. Closed in favour of this.

The actual root cause

The macro has two independent causes, and it is worth separating them:

  • (a) the test writes PARAM.input / PARAM.sys, which are private,
    because the code under test reads global PARAM itself and there is no other
    way to drive it;
  • (b) the test reaches into private members of the class under test.

Passing INPUT values explicitly fixes (a) and nothing else. Where (a) was the
only reason, the macro disappears outright. Where (b) is also present, the
macro stays but now covers only (b), which is a smaller and honest scope.

Evidence that these really are separate: source_cell/test/klist_test.cpp went
through exactly this refactor in #7644read_kpoints(ucell, k_file) became
read_kpoints(ucell, k_file, gamma_only_local, kspacing, kmesh_type, koffset)
— and its PARAM.input writes went 68 -> 6 -> 0 while the macro never moved,
because that file's macro was always about (b).

source_relax, converted

source_relax is the module where (a) dominates and the reads are contained,
so it is done here in full. It reads force_thr, force_thr_ev, stress_thr,
fixed_ibrav, out_level and test_relax_method from PARAM in fifteen
places across seven functions. Those now arrive as arguments:

  • leaf functions take the individual values they use — move_atoms(..., test_relax_method), Ions_Move_Basic::check_converged(..., force_thr, force_thr_ev, out_level, test_relax_method),
    Lattice_Change_Basic::check_converged(..., stress_thr),
    change_lattice(..., fixed_ibrav), new_step, compute_trust_radius,
    restart_bfgs, bfgs_routine, cal_tradius_sd;
  • the plumbing — Ions_Move_{BFGS,CG,SD}::start,
    Ions_Move_Methods::cal_movement, Lattice_Change_CG::start,
    Lattice_Change_Methods::cal_lattice_change — carries a small
    Relax_Criteria POD so those signatures stay readable;
  • relax_nsync.cpp fills it. That file already held an injected
    const Input_para* and had zero PARAM references, so the settings now enter
    the module at one place instead of fifteen.

Relax_Criteria's field defaults deliberately mirror the corresponding
Input_para defaults (force_thr = -1, force_thr_ev = -1,
stress_thr = 0.5, out_level = "ie"). This matters more than it looks: several
of these tests assert on log text that prints a threshold they never set, so they
were implicitly asserting on the INPUT default -- lattice_change_cg_test
expects threshold is 0.5 kbar, and three check_converged cases expect
threshold is -1 eV/Angstrom. Mirroring the defaults keeps the tested behaviour
identical. The first run of this branch failed five suites for exactly this
reason before the defaults were aligned; no expected value was relaxed to make
them pass.

force_thr_ev is passed rather than recomputed from force_thr on the spot:
ReadInput reconciles the two with the literal factor 13.6058 / 0.529177,
which is not bit-identical to Ry_to_eV / BOHR_TO_A, and these tests compare
log text.

Result: PARAM references in source_relax production code 76 -> 11
(the 11 left are in relax_driver.cpp and the bfgs2/lbfgs variants, which no
test drives through PARAM). Every test in the module is now PARAM-free, and
ions_move_basic_test.cpp and lattice_change_basic_test.cpp no longer need the
macro at all. The others keep it for reason (b) only.

No default arguments were added; every call site is updated explicitly.

Carried over from #7919 (independent of either route)

  • The macro had escaped into the production build.
    source_io/module_output/cal_test.cpp is not a test despite the name — it is
    compiled into the io_input library from source_io/CMakeLists.txt — and it
    carried #define private public, so the shipped binary contained one
    translation unit whose view of Parameter disagreed with every other. It only
    reads PARAM.inp / PARAM.globalv, so the macro was dead weight.
  • Undefined behaviour removed. 17 files never closed the region, so the
    macro also reinterpreted access control for whatever followed — <omp.h>,
    <mpi.h>, <string>, <fstream>, scalapack_connector.h (10 includes
    across 7 files). Every region now ends in a matching #undef and contains
    only project headers, with #ifdef _OPENMP / #ifdef __MPI guards intact.
  • Dead code. source_hsolver/test/test_diago_assist.cpp is in no
    CMakeLists.txt, includes a header that does not exist
    (diago_iter_assis.h; the real one is diago_iter_assist.h), declares a
    statement inside a class body and calls EXPECT_EQ with one argument — it has
    never been compiled. source_io/test/for_testing_input_conv.h is included by
    nothing. Both deleted. test_hsolver.cpp keeps the macro for uses that are
    all commented out; dropped.
  • CI ratchet. check_access_hacks() uses the same added-minus-deleted
    budget as the existing global-dependency check: removals are free, a net
    increase is an error. Verified against a synthetic violation
    ([ERROR], exit 1). AGENTS.md rule 10 previously stated the prohibition
    with no mechanical check behind it; it now names the root cause and the fix.

Result

before after
files with the macro 81 70
occurrences 111 98
in a production (non-test) directory 1 0
regions reaching a system header 7 0
regions leaking to end of TU 17 0
PARAM refs in source_relax production code 76 11

Governance budget for this PR: PARAM/GlobalV added=3, removed=67,
net_delta=-64.

What is deliberately not here

The other 70 files split into two groups.

Reason (b) — the test reads private state — needs 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 the result
(BFGS_Basic::check_wolfe_conditions sets private wolfe_flag and the test
asserts on it; returning bool would dissolve that macro and improve the API).
Of 229 private members these tests touch, 50 already have a public getter
that the test simply is not using.

Reason (a) elsewhere is not always this contained: nspin is read 470 times in
production code, nlocal and global_out_dir 143 each. Those are the
de-globalization programme, not one PR.

Several modules also have refactors in flight that would conflict — #7918
touches all six remaining source_md test files plus esolver_lj.h and
setcell.h; #7903 touches vdw_test.cpp, test_init_dm_from_file.cpp and
xc_functional.h. Those are better done inside those PRs, which the ratchet now
makes cheap to ask for in review.

Verification

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

  • build: 0 failed targets
  • ctest unit suite: 335 tests, 29 failed -- the identical set to
    upstream/develop built and run in the same environment.
    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, ...).
  • agent_governance_check.py --base upstream/develop --head HEAD: 0 errors

Governance warnings and why: 10 "Header dependency review" hits are the
#include "relax_criteria.h" lines this change adds to the nine relax headers,
plus <string> inside the new header itself -- unavoidable for a type that
appears in those signatures. relax_criteria.h is a header-only POD, so no
CMakeLists.txt change is needed (source_relax/CMakeLists.txt lists only
.cpp files). The one "Global dependency budget" hit is the doc comment in that
same header naming PARAM; the PR's net delta is -64.

No thresholds changed, no result.ref regenerated.

🤖 Generated with Claude Code

Critsium-xy and others added 5 commits September 7, 2026 16:40
…ng PARAM

The relaxation algorithms read `PARAM.inp.force_thr`, `force_thr_ev`,
`stress_thr`, `fixed_ibrav`, `out_level` and `test_relax_method` directly out of
the global Parameter singleton. That is why their unit tests had to write to
`PARAM.input`, which is private, which is why they carried
`#define private public`.

Thread those values through instead:

- leaf functions take the individual values they use -- `move_atoms(...,
  test_relax_method)`, `check_converged(..., force_thr, force_thr_ev,
  out_level, test_relax_method)`, `Lattice_Change_Basic::check_converged(...,
  stress_thr)`, `change_lattice(..., fixed_ibrav)`,
  `compute_trust_radius(..., test_relax_method)`, `restart_bfgs`,
  `bfgs_routine`, `cal_tradius_sd`;
- the plumbing (`Ions_Move_{BFGS,CG,SD}::start`, `Ions_Move_Methods::cal_movement`,
  `Lattice_Change_CG::start`, `Lattice_Change_Methods::cal_lattice_change`)
  carries a small `Relax_Criteria` POD so those signatures stay readable;
- `relax_nsync.cpp` fills it. That file already held an injected
  `const Input_para*` and had zero PARAM references, so the settings now enter
  the module at one place instead of being read at fifteen scattered ones.

`force_thr_ev` is passed rather than derived from `force_thr` in place: ReadInput
reconciles the two with the literal factor 13.6058/0.529177, which is not
bit-identical to `Ry_to_eV / BOHR_TO_A`, and the tests compare log text.

PARAM references in source_relax production code: 76 -> 11 (the 11 left are in
relax_driver.cpp and the two bfgs2/lbfgs variants, which no test drives through
PARAM). The module's tests no longer touch PARAM at all, and two of them --
ions_move_basic_test.cpp and lattice_change_basic_test.cpp -- no longer need
`#define private public`.

No default arguments were added; every call site is updated explicitly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- BFGS_Basic::new_step also needs test_relax_method, for the
  compute_trust_radius call it makes;
- the tests that drive restart_bfgs / bfgs_routine / cal_tradius_sd /
  compute_trust_radius directly, and the Lattice_Change_CG::start mock in
  lat_change_method_test.cpp, take the new arguments;
- IonsMoveMethodsTest and LatticeChangeCGTest gained the Relax_Criteria member
  their call sites use.

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

Independent of the source_relax refactor, and carried over from the earlier
attempt:

- close every remaining `#define private public` region with a matching
  `#undef` (17 files had none, so the macro reached the end of the translation
  unit) and move system/external includes out of the region (`<omp.h>`,
  `<mpi.h>`, `<string>`, `<fstream>`, scalapack_connector.h -- 10 includes
  across 7 files), keeping their `#ifdef` guards. Rewriting access control
  inside libstdc++ or OpenMPI is undefined behaviour that shows up at link or
  run time rather than as a compile error.
- `source_io/module_output/cal_test.cpp` is not a test despite the name: it is
  compiled into the io_input library from source_io/CMakeLists.txt, so the macro
  had escaped into the production build. It only reads PARAM.inp/globalv, so the
  macro was dead weight there.
- delete `source_hsolver/test/test_diago_assist.cpp` (in no CMakeLists.txt,
  includes a header that does not exist, calls EXPECT_EQ with one argument -- it
  has never compiled) and `source_io/test/for_testing_input_conv.h` (included by
  nothing); drop the vestigial macro in test_hsolver.cpp, whose covered uses are
  all commented out.
- `check_access_hacks()` ratchets on the macro with the same added-minus-deleted
  budget as the global-dependency check: removals free, net increase blocks.
- AGENTS.md rule 10 now names the root cause and the fix.

Also fixes lj_pot_test.cpp, where parameter.h must stay inside the region
because setcell.h writes PARAM.sys from within it.

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

The first test run showed five source_relax suites failing. All five came from
the same mistake: Relax_Criteria's field defaults did not match the Input_para
defaults, so a test that had never set a value no longer got the value it used
to read out of PARAM.

- force_thr_ev defaults to -1 and stress_thr to 0.5, exactly as in
  input_parameter.h. The tests assert on log text that contains these
  thresholds, and several of them deliberately never set one -- e.g.
  lattice_change_cg_test expects 'threshold is 0.5 kbar', the INPUT default.
- ions_move_basic_test no longer derives force_thr_ev from force_thr. It never
  set force_thr_ev, so the faithful value is the -1 default; deriving it changed
  the logged threshold.
- bfgs_test compared bfgs.maxstep against PARAM.inp.relax_bfgs_rmax. Replacing
  that with Ions_Move_Basic::relax_bfgs_rmax was wrong -- it is a different
  variable. PARAM.inp is the public read-only view and was never the problem, so
  the comparison is restored.

No expected value was relaxed to make a test pass; the defaults were corrected
so the tested behaviour is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI caught what the local build could not: source_io/module_json is only added
with ENABLE_RAPIDJSON, so MODULE_IO_JSON_OUTPUT_TEST was never compiled here.

para_json_test.cpp drives a local `Parameter param;` and writes `param.sys` /
`param.input`. The scoping pass hoisted parameter.h out of the macro region
because its 'does this file write PARAM' test only matched the uppercase
`PARAM.input.` singleton, not a lowercase local object -- the same miss already
fixed for lj_pot_test.cpp. Restored inside the region.

Reproduced and verified on a minimal translation unit: the hoisted arrangement
gives exactly the two 'is private within this context' errors CI reported, and
the restored one compiles clean.

Also drops the relax_criteria.h include from the three headers that do not
name the type (ions_move_bfgs2.h, ions_move_lbfgs.h, lattice_change_basic.h).

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

Copy link
Copy Markdown
Collaborator Author

Pushed a fix for the CI failure.

What broke. source_io/module_json is only added when ENABLE_RAPIDJSON=ON, so MODULE_IO_JSON_OUTPUT_TEST was never compiled in my local configuration. para_json_test.cpp drives a local Parameter param; and writes param.sys / param.input; the scoping pass had hoisted parameter.h out of the macro region because its "does this file write PARAM" test only matched the uppercase PARAM.input. singleton, not a lowercase local object. Restored inside the region, reproduced and verified on a minimal translation unit (the hoisted arrangement gives exactly the two is private within this context errors CI reported; the restored one compiles clean).

I also re-checked every call site of the eleven signatures this PR changes across the whole tree, including feature-guarded code: all of them are inside source_relax and all are updated. And I diffed the set of changed test files against the objects my local build actually produced -- para_json_test.cpp was the only one never compiled, the other two are the dead files this PR deletes.

One correction to the table above. "regions reaching a system header: 7 -> 0" is true for direct #include <...> lines inside a region, which is what the scoping pass fixed. It is not true transitively: while reproducing this failure I found that para_json_test.cpp's region reaches <sstream> through abacusjson.h, and compiling that region without gtest's earlier include of <sstream> fails with

/usr/include/c++/12/sstream:447:7: error: 'struct std::__cxx11::basic_stringbuf<...>::__xfer_bufptrs'
    redeclared with different access

That file only builds because gtest happens to be included first and sets the include guard. It is pre-existing on develop and this PR does not make it worse, but it is a concrete instance of the hazard the PR describes, and it means "no region reaches a system header" needs the word directly. Catching the transitive case needs a preprocessor-level check rather than a grep over include lines; I would rather do that separately than widen this PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant