Refactor: structured INPUT parameter availability (Phase 1) - #7783
Refactor: structured INPUT parameter availability (Phase 1)#7783Stardust0831 wants to merge 9 commits into
Conversation
The Input_Item availability field mixes prose and ad-hoc conditions as free text, which cannot be consumed programmatically for tree-structured docs, validation or agent tooling. Add a small dependency-free parser (tools/03_code_analysis/availability_parser.py) that normalises the historical spellings (==, =, 'is set to', 'contains') onto a canonical form and classifies each value as an Expression, a bare Label, or Unstructured prose waiting for review. - availability_parser.py: parse_availability() -> Availability - test_availability_parser.py: unit tests - generate_input_main.py: add --check-availability to report the Expression/Label/Unstructured distribution without changing the generated markdown. On the current docs/parameters.yaml this classifies 217 non-empty values as 82 Expression / 84 Label / 51 Unstructured.
9472f73 to
2050b54
Compare
Make the Input_Item availability field a concrete, machine-readable boolean
condition (single source of truth) so the INPUT docs, validation and tooling
can consume the actual condition instead of free text or bare tags.
- Add AvailabilityKind/AvailabilityCondition/AvailabilityExpr and
parse_availability() in a new module (availability.{h,cpp}, wired into CMake).
- Input_Item now carries availability_kind + availability_expr and a
set_availability() helper that keeps the canonical string and the structured
form in sync (single source of truth).
- Rewrite all 217 non-empty availability registrations to canonical boolean
syntax; the exported YAML classifies them as 216 Expression / 0 Unstructured.
- Export the structured fields from --generate-parameters-yaml (input_help.cpp).
- Consume the structured fields in generate_input_main.py; extend
availability_parser.py to the canonical grammar and cover it with tests.
- Regenerate docs/parameters.yaml and input-main.md.
Former bare "label"-style tags (e.g. "OFDFT", "Numerical atomic orbital basis")
are now expressed as concrete conditions (esolver_type==ofdft, basis_type==lcao,
...) so they can be evaluated by validation/error tooling. mixing_tau has no hard
gate, so its availability is empty (always available); its meta-GGA relevance is
kept in the description.
2050b54 to
8b5d337
Compare
|
I think this is a clever direction for making INPUT parameter constraints more structured and eventually improving consistency checks, but it may still be too early to merge in its current form. There are several design details that should be clarified first, such as how invalid or non-canonical Given the scale and long-term impact of this change, I suggest preparing a short describing the intended design principles, grammar, invariants, and future validation workflow, and sharing it in the WeChat group “abacus开发信息对齐” for broader discussion before proceeding. It would also be helpful to update the developer documentation, including the corresponding Chinese guide (for example, something similar to https://mcresearch.github.io/abacus-user-guide/develop-addinp3.html), so that future contributors clearly understand the new conventions for defining Maybe you can have a look as well @ZhouXY-PKU ? |
The strict C++ parse_availability() in Input_Item::set_availability() is now the single source of truth for the availability grammar (it throws on any non-canonical non-empty string). The legacy python classifier, its unit tests, and the --check-availability report plumbing in generate_input_main.py were an earlier design step and are no longer referenced by any workflow; remove them to keep a single grammar implementation.
|
I fully support the intent; the implementation is questionable in some place. Please allow me some time to have a deeper look. |
Growl1234
left a comment
There was a problem hiding this comment.
Thank you for your dedication! Most likely not all review comments are right, so correct me if I made mistakes or missed anything :)
|
|
||
| - **Type**: Integer | ||
| - **Availability**: *Used only for nscf calculations with plane wave basis set.* | ||
| - **Availability**: *calculation==nscf and basis_type==pw* |
There was a problem hiding this comment.
It would be better to use
- **Availability**: *calculation==nscf && basis_type==pw*Or
- **Availability**: *`calculation==nscf`* and *`basis_type==pw`*Similar to or.
There was a problem hiding this comment.
Updated availability expressions to render as inline code.
| return value.substr(begin, end - begin); | ||
| } | ||
|
|
||
| std::vector<std::string> split_slash_values(const std::string& value) |
There was a problem hiding this comment.
param==a/b effectively introduces another spelling of membership in addition to param in [a, b], while also making / impossible to represent literally in an equality value. I'd suggest keeping == strictly single-valued and using in [...] whenever multiple alternatives are intended.
There was a problem hiding this comment.
Removed the param==a/b special case. == now accepts one value, and alternatives use in [...].
| std::string availability; ///< availability conditions (empty if always) | ||
| /// Set and validate the canonical availability expression. An empty value | ||
| /// means that the item is always available. | ||
| void set_availability(const std::string& value) |
There was a problem hiding this comment.
This currently normalizes non-canonical input rather than actually enforcing canonical input.
For example, anything accepted by the parser can be stored as parsed.to_string(), even if the original registration was not canonical. If the intended invariant is that C++ registrations themselves use the canonical grammar, could we explicitly reject a non-empty value when it differs from parsed.to_string()?
That would catch malformed or non-canonical metadata at development time instead of silently fixing it.
There was a problem hiding this comment.
set_availability() now rejects non-canonical strings instead of normalizing them.
| item.default_value = "0.5"; | ||
| item.unit = ""; | ||
| item.availability = "Only used when relax_method is cg 2"; | ||
| item.set_availability("relax_method in [cg 2]"); |
There was a problem hiding this comment.
I'm not sure the semantics are well-defined here. relax_method is a Vector of string, while the parser tests explicitly distinguish vector containment (contains) from scalar membership (in). What exactly should relax_method in [cg 2] evaluate against: the whole vector, its first element, or one of its elements? Apparently this will cause confusion.
There was a problem hiding this comment.
The vector-valued input form was introduced in PR #6517.
There was a problem hiding this comment.
My concern is not why relax_method is vector-valued — #6517 explains that part. The question is how the new availability DSL defines the semantics of comparing such a vector. For example, what exactly does relax_method in [cg 2] mean? Does in compare the complete vector [cg, 2], test whether one of its elements belongs to the RHS set, or compare some serialized representation such as "cg 2"? This matters especially because this PR already defines contains specifically for vector containment. I think the DSL should make vector equality/membership unambiguous rather than relying on the historical INPUT spelling.
There was a problem hiding this comment.
Updated the DSL: complete multi-token values are quoted and compared with ==, so this is now relax_method=="cg 2". in [...] is reserved for two or more alternatives; contains remains element containment.
| item.default_value = "1"; | ||
| item.unit = ""; | ||
| item.availability = "method_sto = 2 and out_dos = 1 or cal_cond = True"; | ||
| item.set_availability("(method_sto==2 and out_dos==1) or cal_cond==true"); |
There was a problem hiding this comment.
method_sto itself is only applicable when esolver_type==sdft but that prerequisite is not represented here. If availability expressions are evaluated independently, the default value of method_sto could make this condition true outside SDFT. Therefore, please double-check if dependent options are well encapsulated; if so then this might not be an issue.
There was a problem hiding this comment.
Updated dependent availability expressions to include their enclosing prerequisites, such as esolver_type==sdft.
| AvailabilityExpr parse_condition() | ||
| { | ||
| AvailabilityExpr result; | ||
| result.condition.param = parse_identifier(); |
There was a problem hiding this comment.
The parser currently validates only the syntax of the parameter name. Where will we validate that the referenced parameter actually exists, and that the operator/value are compatible with its declared type?
There was a problem hiding this comment.
Added post-registration validation for referenced labels, operator/type compatibility, and literals.
| if(BUILD_TESTING) | ||
| if(ENABLE_MPI) | ||
| add_subdirectory(test) | ||
| add_subdirectory(test_serial) |
There was a problem hiding this comment.
Note here, test_serial is guarded with ENABLE_MPI. You might have added the unittests in the wrong place, as the new availability parser is (and must be) dependency-free.
There was a problem hiding this comment.
Moved the availability tests outside ENABLE_MPI; they now build independently of MPI.
|
To improve readability, parameter names in the generated availability expressions are now linked to their corresponding parameter descriptions. For example, users can directly follow basis_type or ks_solver to see their meanings and usage, while the availability field remains a pure condition expression for parsing and validation. |
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.source/changes.Linked Issue
Ref #7719 — normalize INPUT
availabilityinto a machine-readable form and align it with parameter validation.Unit Tests and/or Case Tests for my changes
source/source_io/test_serial/availability_test.cpp, wired into the test build asMODULE_IO_availability_serial(run viactest -R MODULE_IO); covers leaf /in-set /contains/and-ornesting with parentheses / round-trip / empty / invalid-input rejection (incl. prose like"Only used for plane wave basis.").abacus --generate-parameters-yaml— COMPLETED; regenerateddocs/parameters.yamlandinput-main.md(generated output byte-consistent with the checked-in docs).docs/parameters.yamlhas 526 params / 216 non-emptyavailability, all concrete booleanExpression(0Label/ 0Unstructured).What's changed?
Make the
Input_Itemavailabilityfield a single-source, machine-evaluable boolean condition so the INPUT docs, validation and tooling can consume and evaluate it instead of re-parsing free text or bare tags.source/source_io/module_parameter/availability.{h,cpp}(wired into CMake) withAvailabilityCondition/AvailabilityExprandparse_availability()(operators==/!=/>/>=/</<=/in/contains, combinatorsand/or/,,(...)grouping).Input_Itemkeeps the canonicalavailabilitystring plusavailability_expr(parsed tree) and aset_availability()helper keeping both in sync (single source of truth); the rawavailabilitystring is now private, so the only way to set it is through the grammar-validatingset_availability().OFDFT,Numerical atomic orbital basis, ...) are now concrete conditions over existing parameters (esolver_type==ofdft,basis_type==lcao,calculation==gen_bessel,dft_plus_u==1, ...). Vector parameters usecontains(e.g.td_ttype contains 2) to preserve containment semantics.--generate-parameters-yamlnow emits the canonicalavailabilitystring (empty string = always available) and nothing else, keeping the YAML lean; the structured parse tree stays in C++ for a future validation/error layer.docs/parameters.yamlandinput-main.md.Manual fidelity: rewrites preserve the manual's original meaning. Where the manual states a gate the code does not enforce via an INPUT boolean, it is called out in Governance Notes.
Governance Notes
docs/parameters.yamlanddocs/advanced/input_files/input-main.mdregenerated from the new binary (per AGENTS.md). Full old→new mapping was reviewed locally.source/source_io/module_parameter/input_item.hgains anavailability_exprmember and the newavailabilitymodule; metadata-only, no runtime/numerical behavior change.input_item.hincludesavailability.hbecauseInput_ItemholdsAvailabilityExprby value (a forward declaration is not possible);availability.hincludes<string>/<vector>for its members (std::string,std::vector). Required includes, not reducible to forward declarations.mixing_dmr: manual>= 0.0(always true) kept asmixing_restart>=0; esolver checks> 0— likely loose manual wording.mixing_tau: "Only relevant for meta-GGA" is a note decided at runtime (XC_Functional::get_ked_flag), not an INPUT gate; availability empty, note kept in description.fixed_ibrav: encoded asrelax_method in [cg 2] and latname != none(both manual clauses; the latter also enforced bycheck_value).