From b22305500905da49d790dfff26eaebaff2df94aa Mon Sep 17 00:00:00 2001 From: Zanthoxylum <105619360+Zanthoxylum@users.noreply.github.com> Date: Fri, 30 Oct 2026 23:43:45 +0800 Subject: [PATCH 1/2] refactor(input): declare dfpt_mix_type/a2/ndim domains (set_choices/set_rule) Follow-up to PR #8214 review finding A10: replace the hand-written check_value lambdas with the declarative domain system so the constraints appear in the generated help and rejection routes through the shared input-error path. - dfpt_mix_type: domain.set_choices({rho_plain, rho_kerker, rho_broyden, v_sc_plain, v_sc_broyden}) - dfpt_kerker_a2: domain.set_rule("value>0") - dfpt_broyden_ndim: domain.set_rule("value>=1") No behavior change; DFPT serial 9/9, INPUT domain/reader/help tests all pass. MODULE_IO_input_test_para fails identically on the develop baseline (pre-existing MPI environment issue). --- .../module_parameter/read_inp_dfpt.cpp | 62 +++++++------------ 1 file changed, 22 insertions(+), 40 deletions(-) diff --git a/source/source_io/module_parameter/read_inp_dfpt.cpp b/source/source_io/module_parameter/read_inp_dfpt.cpp index a673bf2548c..a2e93fe84f5 100644 --- a/source/source_io/module_parameter/read_inp_dfpt.cpp +++ b/source/source_io/module_parameter/read_inp_dfpt.cpp @@ -226,28 +226,20 @@ void ReadInput::item_dfpt() "v_sc_broyden is the default (the metric the Broyden least " "squares works in is K-weighted, which stabilizes the " "Coulomb-stiff shells that destabilize plain density mixing)."; - item.default_value = "v_sc_broyden"; - item.read_value = [](const Input_Item& item, Parameter& para) { - if (item.get_size() == 0) - { - para.input.dfpt_mix_type = "v_sc_broyden"; - } - else - { - para.input.dfpt_mix_type = strvalue; - } - }; - sync_string(input.dfpt_mix_type); - item.check_value = [](const Input_Item& item, const Parameter& para) { - const std::string& mt = para.input.dfpt_mix_type; - if (mt != "rho_plain" && mt != "rho_kerker" && mt != "rho_broyden" - && mt != "v_sc_plain" && mt != "v_sc_broyden") - { - ModuleBase::WARNING_QUIT("ReadInput", - "dfpt_mix_type must be rho_plain, rho_kerker, rho_broyden, v_sc_plain or v_sc_broyden."); - } - }; - this->add_item(item); + item.default_value = "v_sc_broyden"; + item.domain.set_choices({"rho_plain", "rho_kerker", "rho_broyden", "v_sc_plain", "v_sc_broyden"}); + item.read_value = [](const Input_Item& item, Parameter& para) { + if (item.get_size() == 0) + { + para.input.dfpt_mix_type = "v_sc_broyden"; + } + else + { + para.input.dfpt_mix_type = strvalue; + } + }; + sync_string(input.dfpt_mix_type); + this->add_item(item); } { Input_Item item("dfpt_kerker_a2"); @@ -258,15 +250,10 @@ void ReadInput::item_dfpt() "by dfpt_mix_type = rho_kerker. The screen f_g = |G+q|^2 / " "(|G+q|^2 + a^2) freezes the stiff long-wavelength shells; " "larger a^2 damps more shells. Ignored by the other schemes."; - item.default_value = "1.0"; - read_sync_double(input.dfpt_kerker_a2); - item.check_value = [](const Input_Item& item, const Parameter& para) { - if (para.input.dfpt_kerker_a2 <= 0.0) - { - ModuleBase::WARNING_QUIT("ReadInput", "dfpt_kerker_a2 must be > 0."); - } - }; - this->add_item(item); + item.default_value = "1.0"; + item.domain.set_rule("value>0"); + read_sync_double(input.dfpt_kerker_a2); + this->add_item(item); } { Input_Item item("dfpt_broyden_ndim"); @@ -281,15 +268,10 @@ void ReadInput::item_dfpt() "and the density mixer of the electric-field " "(LO-TO) loop. More history generally accelerates " "convergence at the cost of memory."; - item.default_value = "8"; - read_sync_int(input.dfpt_broyden_ndim); - item.check_value = [](const Input_Item& item, const Parameter& para) { - if (para.input.dfpt_broyden_ndim < 1) - { - ModuleBase::WARNING_QUIT("ReadInput", "dfpt_broyden_ndim must be >= 1."); - } - }; - this->add_item(item); + item.default_value = "8"; + item.domain.set_rule("value>=1"); + read_sync_int(input.dfpt_broyden_ndim); + this->add_item(item); } } } // namespace ModuleIO From fd271f168aebe19dae98fdbf674fcb78057bd962 Mon Sep 17 00:00:00 2001 From: Zanthoxylum <105619360+Zanthoxylum@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:01:59 +0800 Subject: [PATCH 2/2] fix(input): drop unreachable empty-value fallback for dfpt_mix_type; add reader tests The scalar choices domain now rejects an explicitly empty dfpt_mix_type before read_value runs, so the legacy get_size()==0 fallback (which silently mapped an empty value onto v_sc_broyden) was unreachable. Replace the hand-written read_value with read_sync_string and record the contract in a comment: omitted line keeps the default, an explicit empty or unknown value is rejected. Add reader tests for the three registrations: all five dfpt_mix_type choices plus empty/unknown/multi-token rejection, dfpt_kerker_a2 with the a^2=0 boundary under value>0, and dfpt_broyden_ndim with the ndim=0 boundary under value>=1. Each rejection also checks that read_value is not reached (previous value survives) and the doc anchor. --- .../module_parameter/read_inp_dfpt.cpp | 18 +-- .../test_serial/read_input_item_test.cpp | 128 ++++++++++++++++++ 2 files changed, 135 insertions(+), 11 deletions(-) diff --git a/source/source_io/module_parameter/read_inp_dfpt.cpp b/source/source_io/module_parameter/read_inp_dfpt.cpp index a2e93fe84f5..5b72fbbabbc 100644 --- a/source/source_io/module_parameter/read_inp_dfpt.cpp +++ b/source/source_io/module_parameter/read_inp_dfpt.cpp @@ -228,17 +228,13 @@ void ReadInput::item_dfpt() "Coulomb-stiff shells that destabilize plain density mixing)."; item.default_value = "v_sc_broyden"; item.domain.set_choices({"rho_plain", "rho_kerker", "rho_broyden", "v_sc_plain", "v_sc_broyden"}); - item.read_value = [](const Input_Item& item, Parameter& para) { - if (item.get_size() == 0) - { - para.input.dfpt_mix_type = "v_sc_broyden"; - } - else - { - para.input.dfpt_mix_type = strvalue; - } - }; - sync_string(input.dfpt_mix_type); + // Contract: the line may be omitted (default_value v_sc_broyden + // applies) or supply exactly one of the five choices. An + // explicitly empty or unknown value is rejected by the scalar + // choices domain before read_value runs; the previous + // get_size()==0 fallback silently mapped an empty value onto + // v_sc_broyden and is intentionally dropped. + read_sync_string(input.dfpt_mix_type); this->add_item(item); } { diff --git a/source/source_io/test_serial/read_input_item_test.cpp b/source/source_io/test_serial/read_input_item_test.cpp index b1221ff0b77..b549d2fca53 100644 --- a/source/source_io/test_serial/read_input_item_test.cpp +++ b/source/source_io/test_serial/read_input_item_test.cpp @@ -2816,3 +2816,131 @@ TEST_F(InputTest, PerAtomSccsCavityWidthsDisableSpatialSymmetry) it->second.reset_value(it->second, param); EXPECT_EQ(input.symmetry, "1"); } + +TEST_F(InputTest, DfptMixTypeDomainContract) +{ + ModuleIO::ReadInput readinput(0); + readinput.check_ntype_flag = false; + Parameter param; + auto it = find_label("dfpt_mix_type", readinput.input_lists); + ASSERT_NE(it, readinput.input_lists.end()); + Input_para& input = TestParameters::input(param); + + // omitted line: the declared default applies (Input_para default and + // item default_value agree) + EXPECT_EQ(it->second.default_value, "v_sc_broyden"); + EXPECT_EQ(input.dfpt_mix_type, "v_sc_broyden"); + + // every declared choice is read verbatim + for (const std::string scheme : {"rho_plain", "rho_kerker", "rho_broyden", "v_sc_plain", "v_sc_broyden"}) + { + it->second.str_values = {scheme}; + it->second.read_value(it->second, param); + EXPECT_EQ(input.dfpt_mix_type, scheme) << scheme; + } + + // explicit empty / unknown / multi-token values are rejected by the + // scalar choices domain before read_value runs, so the previous value + // survives (the legacy get_size()==0 fallback is intentionally gone) + readinput.readvalue_items.push_back(&it->second); + const std::string previous_scheme = input.dfpt_mix_type; + for (const std::vector& invalid : { + std::vector{}, + std::vector{""}, + std::vector{"broyden"}, + std::vector{"v_sc"}, + std::vector{"rho_plain", "v_sc_plain"}}) + { + it->second.str_values = invalid; + const std::string error = readinput.prepare_input(param); + EXPECT_THAT(error, testing::HasSubstr("Invalid INPUT parameter 'dfpt_mix_type'")); + EXPECT_THAT(error, testing::HasSubstr("Expected one String from {rho_plain, rho_kerker, rho_broyden, v_sc_plain, v_sc_broyden}")); + EXPECT_THAT(error, testing::HasSubstr("input-main.html#dfpt-mix-type")); + EXPECT_EQ(input.dfpt_mix_type, previous_scheme); + } +} + +TEST_F(InputTest, DfptKerkerA2RuleBoundaries) +{ + ModuleIO::ReadInput readinput(0); + readinput.check_ntype_flag = false; + Parameter param; + auto it = find_label("dfpt_kerker_a2", readinput.input_lists); + ASSERT_NE(it, readinput.input_lists.end()); + Input_para& input = TestParameters::input(param); + + EXPECT_EQ(it->second.default_value, "1.0"); + EXPECT_DOUBLE_EQ(input.dfpt_kerker_a2, 1.0); + + for (const std::pair valid : { + std::pair{"0.5", 0.5}, + {"1e-2", 1.0e-2}, + {"12", 12.0}}) + { + it->second.str_values = {valid.first}; + it->second.read_value(it->second, param); + EXPECT_DOUBLE_EQ(input.dfpt_kerker_a2, valid.second) << valid.first; + } + + // a^2 = 0 turns the Kerker screen off, so the rule value>0 rejects the + // boundary together with negatives, nonfinite values, and empty input + readinput.readvalue_items.push_back(&it->second); + const double previous_a2 = input.dfpt_kerker_a2; + for (const std::vector& invalid : { + std::vector{}, + std::vector{"0"}, + std::vector{"-1"}, + std::vector{"nan"}, + std::vector{"inf"}, + std::vector{"0.5garbage"}}) + { + it->second.str_values = invalid; + const std::string error = readinput.prepare_input(param); + EXPECT_THAT(error, testing::HasSubstr("Invalid INPUT parameter 'dfpt_kerker_a2'")); + EXPECT_THAT(error, testing::HasSubstr("Expected one finite Real satisfying value>0.")); + EXPECT_THAT(error, testing::HasSubstr("input-main.html#dfpt-kerker-a2")); + EXPECT_DOUBLE_EQ(input.dfpt_kerker_a2, previous_a2); + } +} + +TEST_F(InputTest, DfptBroydenNdimRuleBoundaries) +{ + ModuleIO::ReadInput readinput(0); + readinput.check_ntype_flag = false; + Parameter param; + auto it = find_label("dfpt_broyden_ndim", readinput.input_lists); + ASSERT_NE(it, readinput.input_lists.end()); + Input_para& input = TestParameters::input(param); + + EXPECT_EQ(it->second.default_value, "8"); + EXPECT_EQ(input.dfpt_broyden_ndim, 8); + + // ndim = 1 is the smallest legal history depth (single-step memory) + for (const std::pair valid : { + std::pair{"1", 1}, + {"16", 16}}) + { + it->second.str_values = {valid.first}; + it->second.read_value(it->second, param); + EXPECT_EQ(input.dfpt_broyden_ndim, valid.second) << valid.first; + } + + // ndim = 0 (empty history) is rejected together with negatives, + // non-integers, and empty input + readinput.readvalue_items.push_back(&it->second); + const int previous_ndim = input.dfpt_broyden_ndim; + for (const std::vector& invalid : { + std::vector{}, + std::vector{"0"}, + std::vector{"-4"}, + std::vector{"2.5"}, + std::vector{"8", "1"}}) + { + it->second.str_values = invalid; + const std::string error = readinput.prepare_input(param); + EXPECT_THAT(error, testing::HasSubstr("Invalid INPUT parameter 'dfpt_broyden_ndim'")); + EXPECT_THAT(error, testing::HasSubstr("Expected one Integer satisfying value>=1.")); + EXPECT_THAT(error, testing::HasSubstr("input-main.html#dfpt-broyden-ndim")); + EXPECT_EQ(input.dfpt_broyden_ndim, previous_ndim); + } +}