umat: define models in JSON, with the deck's constants bound into the graph - #30
umat: define models in JSON, with the deck's constants bound into the graph#30petlenz wants to merge 15 commits into
Conversation
… graph
A builder written as a C++ lambda means recompiling the shared library for
every new material, which defeats the point of the deck driving the model.
make_json_builder turns a JSON document into the same builder the registry
already takes, so a new material is a config edit.
The document is the one io/json_material_factory already understands, plus a
"constants" array binding *USER MATERIAL, CONSTANTS= to named parameters:
"constants": ["K::value", "G::value"]
Named "constants" rather than "props" because a PROPERTY here is a graph node,
and reusing that word for the deck's numbers names two unrelated things the
same. Targets use the library's existing "material::parameter" syntax and go
through connection_source::parse rather than inventing a second spelling.
Any unrecognised top-level key is rejected. Ignoring one meant a document
still saying "props" was accepted with every constant UNBOUND, leaving the
placeholder zeros in the document as the material's moduli -- wrong, plausible
and silent. json_to_parameters already warns per material; this is the same
check one level up.
nlohmann/json moves from an optional __has_include probe to a declared
dependency with a FetchContent fallback. Configuration is meant to be JSON
driven, so a build without it is missing the primary way to define a material,
not an extra -- and the probe meant the JSON tests silently vanished on a
machine without it installed.
7 tests, driven through the real umat_ entry point rather than the C++
evaluator, so the whole path from Fortran ABI to graph is covered.
petlenz
left a comment
There was a problem hiding this comment.
Critical review — probed rather than read. Two findings, one of them high. Both are the same shape as the unknown-top-level-key check this PR already adds, but one level further down: a target that validates halfway and then silently does nothing.
| # | severity | finding |
|---|---|---|
| 1 | high | a typo in the PARAMETER half of a "constants" target is accepted; the material keeps its placeholder |
| 2 | medium | a duplicate "constants" target silently drops a deck constant |
Details inline. Happy to fix both.
| throw fatal_error( | ||
| "json_model: every \"constants\" entry must be a string"); | ||
| auto binding = parse_constant_target(entry.get<std::string>()); | ||
| const bool known = std::any_of( |
There was a problem hiding this comment.
Finding 1 (high): the binding target is only half validated.
This checks that binding.material names a material the document defines. Nothing checks binding.property. The substitution below is material[bindings[i].property] = props[i] on a nlohmann::json object — which creates the key rather than failing, so a typo writes a parameter no material declares and leaves the real one at its placeholder.
"constants": ["K::vlaue", "G::value"] with placeholders K=7, G=3, through the real umat_ entry point:
warning: unknown parameter 'vlaue' in JSON (not in schema)
(A) accepted. C1111=127.0000 deck wanted 370.0000 placeholder gives 11.0000 fatals=0
127 = 7 + 4(90)/3. G took the deck value, K kept the document's placeholder. No fatal, no cutback — just a stderr warning that in a real Abaqus job is buried in thousands of lines of solver output, if it is captured at all.
The comment three lines up says:
Validate the props bindings now rather than on first use, so a typo is reported when the model is registered rather than mid-analysis.
That is true for the material half and false for the parameter half, which is worse than not claiming it.
The fix wants the target material's declared parameter set. parameters() is static per material type, and the factory already knows the type from "type" — so registration can ask the schema whether the parameter exists, the same way json_to_parameters does when it emits that warning. Failing that, a cheaper version that still closes it: require the key to already be present in the document, so "K::value" only validates when the material actually writes a "value" placeholder. That turns the placeholders from a convention into the declaration, which is arguably clearer anyway.
Either way this needs a test with a misspelled parameter.
There was a problem hiding this comment.
Fixed. Targets are now checked against the material's declared parameters, and the error lists what the type does take:
json_model: constants entry 'K::vlaue' names parameter 'vlaue', which
constant_scalar does not declare — it takes: name, value
That needs a populated factory, so register_default_materials moves out of the builder into ensure_materials_registered<Traits>(), which registration and the builder both call. A type the factory does not know is skipped rather than rejected — otherwise a document could no longer name a material the caller registers after this one, and the type is caught at build time anyway.
Paired with the near-miss ("K::value" still registers) so the check can't pass by rejecting everything.
Also made the claim in the comment true: it now says both halves are validated, because now they are.
| if (!known) | ||
| throw fatal_error("json_model: constants entry targets material '" + | ||
| binding.material + | ||
| "', which the document does not define"); |
There was a problem hiding this comment.
Finding 2 (medium): a duplicated target silently drops a deck constant.
Nothing rejects the same target twice. "constants": ["K::value", "K::value"] assigns props[0] then props[1] to the same key — last write wins, props[0] is discarded, and G is never bound at all:
(B) accepted. C1111=90.0000 fatals=0
90 = props[1] + 0: K became 90, and G stayed at its placeholder of 0 — a zero shear modulus, silently.
This is a document-authoring mistake rather than a library bug, but it is exactly the class this PR decided to make loud (see the "props" rejection), and it is a two-line check next to the existing known test:
if (!seen.insert(entry.get<std::string>()).second)
throw fatal_error("json_model: constants entry '" + target +
"' appears twice; each host constant binds one target");Worth catching the near-miss too — two entries targeting the same MATERIAL with different parameters is legitimate, so the key has to be the full material::parameter, not just the material.
There was a problem hiding this comment.
Fixed. Duplicate targets are rejected at registration:
json_model: constants entry 'K::value' appears twice; each host constant binds
one target, and a repeat silently drops the earlier one
The key is the whole material::parameter target, as you flagged — the second test registers ["stiffness::K_property", "stiffness::G_property"] to pin that two entries against one material stay legal.
Two review findings on the JSON model layer, both reproduced first. 1. Only the MATERIAL half of a "material::parameter" target was checked. The substitution is material[property] = props[i] on a nlohmann::json object, which CREATES a missing key rather than failing -- so a misspelled parameter was written where nothing reads it while the real one kept the document's placeholder. With "K::vlaue" against a placeholder of 7 and a deck supplying 250, the tangent came back 127 = 7 + 4(90)/3 instead of 370, with no fatal and no cutback: a stderr warning was the only trace, and in a real job that is buried in solver output if it is captured at all. The comment above that check claimed typos were reported at registration. True for the material half, false for the parameter half -- worse than not claiming it, since it reads as though the whole target were verified. Targets are now checked against the material's declared parameters, listing what the type does take. That needs a populated factory, so registration of the default materials moves out of the builder into a helper both call. A type the factory does not know is skipped rather than rejected, so a document stays free to name a material the caller registers afterwards. 2. Nothing rejected the same target twice. ["K::value", "K::value"] assigned props[0] then props[1] to one key -- last write wins, props[0] dropped, and G never bound at all, leaving a shear modulus of zero. The key is the whole target, since two entries against one material with different parameters is legitimate. Four tests, each paired with the near-miss that must still be accepted so the checks cannot pass by rejecting everything.
petlenz
left a comment
There was a problem hiding this comment.
Second review, after the fixes. The two earlier findings are closed — re-ran both probes and each is now rejected at registration with a message naming the problem.
One gap remains in the new check itself.
| // free to name a material the caller registers later. | ||
| if (!factory.contains(type)) return; | ||
|
|
||
| const auto wanted = bound_parameter(material, binding); |
There was a problem hiding this comment.
The check verifies the parameter is DECLARED, not that it can meaningfully take a number.
"name" is declared by every material, via base::parameters(). So this passes:
"constants": ["K::name", "G::value"]registration: ACCEPTED
FATAL: building the material graph for 'NAMEBIND' failed:
[json.exception.type_error.302] type must be string, but is number
Two things wrong with that, neither fatal:
- It is deferred to the first UMAT call, which is what this validation exists to avoid — the whole point of checking at registration is that a config fault surfaces before an analysis starts.
- The message is a raw nlohmann type error. It says a string was expected where a number arrived; it does not say a host constant cannot be bound to a material's name.
Cheapest fix is rejecting "name" by name, since it is the only universally-declared parameter and binding a deck constant to it is never meaningful. The more general fix is checking the declared parameter's type accepts a double, which the schema's type_index would support — worth it only if other non-numeric parameters turn up as plausible targets.
Low severity: it fails loudly and you have to write something odd to reach it. Flagging it because it is a hole in a check I added specifically to close holes of this shape.
There was a problem hiding this comment.
Fixed — and generalised past the case I reported.
Rejecting "name" alone would not have closed the class: most materials also declare *_source strings, and binding a constant to one of those fails the same way. The check now requires the declared parameter's type to accept a number, and the error lists the ones that do:
constants entry 'K::name' names 'name', which constant_scalar does not take
as a number — a host constant can bind: value
The new check immediately caught a bad example in my own test. RejectsADuplicateConstantsTarget used ["stiffness::K_property", "stiffness::G_property"] as the legitimate two-entries-one-material case — but those are string parameters, so binding numbers to them was never meaningful. Replaced with linear_elasticity's K and G, which really are numeric.
199 tests.
Checking only that the parameter is declared accepts targets that can never work: every material declares "name", and most declare *_source strings. A document binding "K::name" was accepted at registration and failed on the first UMAT call with a raw nlohmann type error -- deferred past the point this validation exists to precede, and saying nothing about decks. The declared parameter's type must now accept a number, and the error lists the ones that do. Caught a bad example in the duplicate-target test on the way: it used "stiffness::K_property" as a legitimate second binding, but those are string parameters and binding a constant to one was never meaningful. Replaced with linear_elasticity's K and G.
Linking nlohmann_json::nlohmann_json unconditionally works only when the package is FOUND installed. On the FetchContent path the target is not in any export set, so install(EXPORT) fails at configure: CMake Error: install(EXPORT "numsim-materialsTargets" ...) includes target "numsim-materials" which requires target "nlohmann_json" that is not in any export set. The repo already had the answer, three lines above, in tmech's handling: add the include path instead of linking, precisely because linking pulls it into the export set. nlohmann now gets the same treatment when fetched, and the plain link when found. Local builds could not surface this -- find_package succeeds on a machine that has the package, so the fetch path never ran here. Reproduced with -DCMAKE_DISABLE_FIND_PACKAGE_nlohmann_json=ON: 199/199.
Stacked on #28. Independent of #29.
A builder written as a C++ lambda means recompiling the shared library for every new material, which defeats the point of the deck driving the model.
make_json_builderturns a JSON document into the same builder the registry already takes, so a new material is a config edit.{ "materials": [ {"type": "external_strain_source", "name": "strain_in"}, {"type": "constant_scalar", "name": "K", "value": 0}, {"type": "constant_scalar", "name": "G", "value": 0}, {"type": "isotropic_tangent", "name": "stiffness", "K_source": "K", "G_source": "G"}, {"type": "linear_stress", "name": "elastic", "tangent_source": "stiffness", "strain_source": "strain_in"} ], "constants": ["K::value", "G::value"] }PROPS[i]replaces the parameter named byconstants[i].Naming
Called
constants, notprops, because a property here is a graph node — reusing that word for the deck's numbers names two unrelated things the same in the one document where both appear. It is also what the deck calls them (*USER MATERIAL, CONSTANTS=).Targets use the library's existing
material::parametersyntax (time::state) and go throughconnection_source::parserather than inventing a second spelling.An unrecognised top-level key is now rejected
Ignoring one meant a document still saying
"props"was accepted with every constant unbound, leaving the placeholder zeros in the document as the material's moduli — wrong, plausible and completely silent.json_to_parametersalready warns about unknown keys per material; this is the same check one level up.nlohmann/json
Moves from an optional
__has_includeprobe to a declared dependency with aFetchContentfallback. Configuration is meant to be JSON driven, so a build without it is missing the primary way to define a material, not an extra — and the probe meant the JSON tests silently vanished on a machine without it installed.Tests
7, driven through the real
umat_entry point rather than the C++ evaluator, so the whole path from Fortran ABI to graph is covered. Validation cases (malformed document, unqualified target, undefined material, unknown top-level key) all fire at registration rather than mid-analysis.Split out of #26.