umat: compare the deck's constants by value, not just by count - #27
umat: compare the deck's constants by value, not just by count#27petlenz wants to merge 10 commits into
Conversation
The per-thread cache keyed a built context on the material name and checked only NPROPS on later calls. Two calls for one name with the same count but different numbers therefore passed the check and were served the FIRST call's graph, whose constants are baked into its parameters. The analysis converges and reports nothing; the moduli are simply wrong from the second call on. Store the constants and compare them. NPROPS doubles is nothing against an evaluation -- 22x an update just to rebuild a graph, so the comparison is not the cost worth saving here. Found while writing tests for the JSON model layer, but the defect is in the registry and independent of it.
…nd model
Two fixes from reviewing the props_scalar PR, both reproduced before fixing.
1. The guard disabled the consistency check for the whole model as soon as ANY
constant was live. A document mixing the two binding times therefore let a
changed BAKED constant through: through the real umat_ entry point, with
K baked and G live and both changed from {100,40} to {300,140}, the tangent
came back 286.667 = 100 + 4(140)/3. G tracked the deck, K silently kept its
first value, no diagnostic. That is the defect #27 exists to catch,
reintroduced for mixed documents.
The check is now per slot: the count still has to match, and each value is
compared unless a props_scalar owns that slot. Mixing is a reasonable thing
to want -- a temperature-dependent modulus beside a fixed yield stress -- so
rejecting mixed documents outright was the worse fix.
2. Nothing distinguished "no live constants" from "live constants, never
bound". An unbound model published zero for every modulus, giving an
all-zero DDSDDE and a host that fails to converge with nothing naming the
cause. evaluate_canonical now throws if the model has readers and
bind_props has never run.
It catches never-bound, not stale: the plane-stress solve runs the graph
repeatedly for one host call and must not re-bind per iterate, so
"deliberately the same" and "forgot to re-bind" are indistinguishable. Under
the registry the gap does not exist -- it binds on every call.
Four tests, each verified to fail against the code it guards: a mixed document
rejecting a changed baked constant, the same document honouring a changed live
one, evaluating before binding, and the plane-stress bind_props forward -- which
had no coverage at all and is the one place binding meets an iterative
evaluator.
petlenz
left a comment
petlenz
left a comment
There was a problem hiding this comment.
Critical review. The fix itself is correct — std::equal's four-iterator overload handles unequal lengths, empty-vs-empty compares equal so a zero-constant material is fine, and exact comparison is right here because the deck values are bit-identical between calls by construction.
One diagnostic regression, inline.
Also flagging a downstream interaction that is not a defect in this PR but changes what merges on top of it: #31 needs this check answered per slot rather than per model, because a document may mix constants baked into the graph with constants read per call. I found and fixed that in #31, and it re-splits this message in the process — so if this PR merges first, expect that follow-up rather than a conflict.
| " constants but this call supplies " + | ||
| std::to_string(props.size()) + | ||
| " — PROPS must be constant for a given material name"); | ||
| "' was built from a different set of " + |
There was a problem hiding this comment.
The message lost information it used to carry.
Before this PR the count case read:
was built from 2 constants but this call supplies 3
Now every case, including a pure count mismatch, reads:
was built from a different set of 2 constants than this call supplies
The caller's count is gone. For the value case that is fine — the counts are equal by then, so stating one covers both. For the count case it drops the more useful half: a user reading NPROPS=3 against a message that only says 2 has to work out which number is theirs.
The check now covers two genuinely different faults with one message, and they want different text — a wrong CONSTANTS= count is a deck error, whereas equal counts with different numbers is a dispatch error. Splitting them also lets the value case name the offending slot, which is the thing a user actually needs:
constant 1 was baked into the graph as 100.000000 but this call supplies 300.000000
This is what #31 ends up doing, for the per-slot reason in the review body — so it can either land here or arrive with that PR. Mentioning it because diagnostics are the entire value of this check: it never changes a result, it only explains one.
There was a problem hiding this comment.
Fixed here rather than deferred to #31, since it is this PR's regression.
Two messages for two faults:
material 'STIFF' was built from 2 constants but this call supplies 3
— NPROPS cannot vary for a given material name
material 'STIFF' constant 1 was baked into the graph as 100.000000
but this call supplies 300.000000
The tests now assert the message contents — both counts in the first, the slot number and both values in the second — rather than that the word "constant" appears somewhere. This check never changes a result, so the message is the entire feature and deserves to be pinned.
#31 has been re-merged on top and keeps its per-slot skip for live constants.
…agrees Folding both faults into one message lost information the old count check carried. It read "was built from a different set of 2 constants than this call supplies" -- the caller's count gone, so a user reading NPROPS=3 has to work out which number is theirs. They are different faults and want different text: a wrong CONSTANTS= count is a deck error, while equal counts with different numbers is a dispatch error. The value case now names the offending slot and both values: material 'STIFF' constant 1 was baked into the graph as 100.000000 but this call supplies 300.000000 This check never changes a result -- it only ever explains one -- so the message is the entire feature. The tests now assert the contents rather than that the word "constant" appears somewhere.
|
Second review: no findings. Re-probed the earlier ones and both are closed — the split messages now carry both counts and the offending slot, and the Findings this round are on #30 (one low) and #31 (one medium). |
1 participant
The per-thread cache keyed a built context on the material name and then checked only
NPROPSon later calls. Two calls for one name with the same count but different numbers pass that check and are served the first call's graph — whose constants are baked into its parameters.The analysis converges and reports nothing. The moduli are simply wrong from the second call on.
NPROPSdoubles is nothing against an evaluation — rebuilding a graph costs 22x an update, so the comparison is not the cost worth saving here.Test
UmatInterface.ChangingPropsValuesForTheSameNameIsFatal, against a material name no other test warms — the check only fires on a cached entry, so a shared name would let test order decide the outcome. Verified to fail without the fix and pass with it.Found while writing tests for the JSON model layer (#26), but the defect is in the registry and independent of it. Split out so it can land on its own.