materials: props_scalar — deck constants read per call instead of baked in - #31
materials: props_scalar — deck constants read per call instead of baked in#31petlenz wants to merge 24 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.
…ked in
constant_scalar copies its number into the graph at construction, which is
right for Abaqus: PROPS is fixed for a material name, and the registry enforces
it. It is wrong for CalculiX, which interpolates the *USER MATERIAL constants
by temperature so they can differ from one call to the next.
props_scalar records only WHICH slot it owns and takes the number from the host
each call. Rebuilding the graph per call would also be correct -- nothing is
retained between calls -- but measures 22x an evaluation even from a
hand-written builder (6.7 us against 302 ns), where reading a slot is free.
bind() dereferences and keeps nothing. A host constants array may be a per-call
temporary, so an implementation that stored `const double*` would read a dead
stack slot on the next call, and usually return the right number anyway because
the slot is commonly reused. There is a test that clobbers the caller's buffer
between bind and update; it fails against the stored-pointer version.
The property is plain and has no update callback, exactly as constant_scalar:
nothing reaches statev_map (so a modulus costs no STATEV slot) and the value is
in place before ctx.update(), so graph ordering cannot affect it.
In a JSON document the position in "constants" is the slot, so the binding is
written exactly as before and only the material type changes:
- {"type": "constant_scalar", "name": "K", "value": 0},
+ {"type": "props_scalar", "name": "K"},
"constants": ["K::value", "G::value"]
The evaluator collects the readers at construction with the dynamic_cast probe
statev_map already uses, rather than taking them from config -- a hand-listed
set is a second thing to keep in step with the graph, and a missed entry is a
silently stale modulus. It range-checks them once per call and reports a short
array as the setup fault it is.
The registry's PROPS-consistency check stands down for these models: a changed
constants array is the point rather than a contradiction, since nothing was
baked into the graph for it to contradict. That check is the merged #27, which
this needs -- without value comparison the guard has nothing to guard.
std::size_t joins the JSON reader registry so "index" survives the document
round trip.
9 tests.
petlenz
left a comment
petlenz
left a comment
There was a problem hiding this comment.
Reviewed my own PR by trying to break it rather than by re-reading it. Three findings; the first is a correctness regression that reintroduces exactly the bug #27 exists to fix, and I'd hold the PR on it. (Posting as COMMENT — GitHub won't let an author request changes on their own PR.)
All three are reproduced, not inferred — the probes are in the comments.
| # | severity | finding |
|---|---|---|
| 1 | high | a document mixing baked and live constants silently keeps the baked ones at their first value |
| 2 | medium | a forgotten bind_props gives a zero tangent with no diagnostic |
| 3 | low | the plane-stress bind_props forward has no test |
Details inline.
| // array is the point rather than a contradiction: nothing was baked into | ||
| // the graph for it to contradict. material_point_evaluator::bind_props | ||
| // does the range check those models actually need. | ||
| if (!it->second.solid->has_live_props() && |
There was a problem hiding this comment.
Finding 1 (high): this guard is too coarse, and it reintroduces the bug #27 fixes.
has_live_props() is true if any material reads its constants per call. It then disables the consistency check for all of them — including constants that really were baked into the graph at build time.
A document may legitimately mix the two:
{"type": "constant_scalar", "name": "K", "value": 0},
{"type": "props_scalar", "name": "G"},
"constants": ["K::value", "G::value"]Ran it through the real umat_ entry point, changing both constants between calls:
call 1 K=100 G=40 C1111 = 153.333 (expected 153.333)
call 2 K=300 G=140 C1111 = 286.667 (expected 486.667)
fatals reported: 0
286.667 = 100 + 4(140)/3. G tracked the deck; K silently kept its first value, and nothing was reported. That is precisely the failure mode #27 exists to prevent, reopened for mixed documents.
The guard needs to be per-slot, not per-model: compare only the constants no props_scalar owns. The evaluator already knows the owned indices — it computes m_props_needed from them — so exposing that set is enough:
for (std::size_t i = 0; i < it->second.props.size(); ++i)
if (!live.contains(i) && it->second.props[i] != props[i])
throw fatal_error(...);Rejecting mixed documents outright would also close it, but it forbids something reasonable — a temperature-dependent modulus alongside a genuinely fixed yield stress — so I'd rather fix the check.
Either way this needs a test with one baked and one live constant. I have none: every test here is all-baked or all-live, which is why I didn't catch it.
There was a problem hiding this comment.
Fixed in 751f6f5.
The check is now per slot rather than per model: the count still has to match either way, and each value is compared unless a props_scalar owns that slot.
for (std::size_t i = 0; i < props.size(); ++i)
if (!it->second.solid->is_live_prop(i) && it->second.props[i] != props[i])
throw fatal_error(... "constant " + std::to_string(i + 1) + " was baked " ...);The evaluator builds a std::vector<bool> of owned slots at construction, from the same reader scan that already computes m_props_needed — so no new traversal.
Kept mixing legal rather than rejecting it: a temperature-dependent modulus beside a fixed yield stress is a reasonable thing to want, and rejecting the document would have closed the hole by forbidding the use case.
Re-ran the original probe against the fix:
call 1 K=100 G=40 C1111 = 153.333 (expected 153.333)
call 2 K=300 G=140 C1111 = 0.000 (expected 486.667)
fatals reported: 1
Zeroed outputs and one fatal, which is the correct fatal path. The error names the offending slot and both values.
Covered by ChangingABakedConstantIsFatalEvenBesideALiveOne and, so the fix can't pass by rejecting everything, ChangingOnlyTheLiveConstantIsHonouredInAMixedDocument. Verified the first fails against the coarse guard.
| /// A no-op unless the model contains props_scalar materials — a model whose | ||
| /// constants were baked in at build time never reaches the loop. | ||
| void bind_props(std::span<const value_type> props) { | ||
| if (m_props_readers.empty()) return; |
There was a problem hiding this comment.
Finding 2 (medium): a forgotten bind_props is a singular tangent with no diagnostic.
The early return is right for baked models, but there is no state saying "this model has readers and none of them have been bound yet". A direct C++ caller that never calls bind_props gets moduli of zero:
(A) no bind_props : C1111=0.000 sigma11=0 -> SINGULAR tangent, no diagnostic
The host then sees an all-zero DDSDDE and fails to converge, with nothing pointing at the cause. PropsScalar.IsZeroBeforeTheFirstBind currently documents this as intended, which I now think is the wrong call — zero is a plausible-looking number for a modulus, and a degenerate one.
The registry path always binds, so this is direct C++ use only. Still cheap to close:
bool m_props_bound{false}; // set in bind_props
// in evaluate()/evaluate_canonical():
if (!m_props_readers.empty() && !m_props_bound)
throw fatal_error("material_point_evaluator: this model reads its "
"constants per call — bind_props() before evaluate()");One branch on a path that already costs ~300 ns. Alternatively initialise to NaN so it propagates loudly instead of quietly zeroing the stiffness, but an explicit error names the mistake.
There was a problem hiding this comment.
Fixed in 751f6f5.
m_props_bound, checked in evaluate_canonical — the single choke point before ctx.update(), so it covers the plane-stress path too:
if (!m_props_readers.empty() && !m_props_bound)
throw fatal_error("material_point_evaluator: this model reads its material "
"constants per call — call bind_props() before evaluating");Deliberately 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 constants" and "forgot to re-bind" are indistinguishable from here. Under the registry that gap doesn't exist — it binds on every call. Said so in the comment rather than leaving it to be discovered.
I left IsZeroBeforeTheFirstBind in place: the initial value is still worth pinning down, it just can no longer reach a host now that the evaluator refuses.
EvaluatingBeforeBindingIsFatal also asserts that binding afterwards works, so the guard isn't a one-way latch. Verified it fails with the check disabled.
| /// Bind the host's constants once for the whole out-of-plane solve. They do | ||
| /// not depend on the iterate, so binding inside the loop would be repeated | ||
| /// work with no effect. | ||
| void bind_props(std::span<const value_type> props) { |
There was a problem hiding this comment.
Finding 3 (low): this forward is untested.
Deleting the body would not fail a single test — every props_scalar test uses the solid path. I did verify by hand that it works:
(B) plane stress : C1111=118.2609 expected E/(1-nu^2)=118.2609
so this is a coverage gap rather than a defect. But it is the one place where binding interacts with an iterative evaluator, which makes it the most interesting path, not the least: if a future change moved the bind inside the out-of-plane loop, nothing here would notice.
Worth one test that drives plane stress with live constants and asserts the condensed E/(1-v^2).
There was a problem hiding this comment.
Fixed in 751f6f5.
PlaneStressUsesTheLiveConstants drives the plane-stress path with live constants and asserts the condensed modulus computed from the constants the host supplied:
const T E = 9 * K * G / (3 * K + G);
const T nu = (3 * K - 2 * G) / (2 * (3 * K + G));
EXPECT_NEAR(ddsdde[0], E / (1 - nu * nu), 1e-9);
EXPECT_NEAR(stress[2], 0.0, 1e-10);Derived rather than hardcoded, so it fails if the constants don't reach the material at all — which is what a deleted forward looks like. Verified: replacing the body with (void)props; fails it.
Also asserts sigma_33 is driven to zero, so the test covers the out-of-plane solve actually converging with host-supplied moduli, not just the tangent value.
| // copied in now and keeps it for the life of the graph. The position in | ||
| // "constants" is the slot either way, so the document says the same | ||
| // thing and only the material type decides when it is read. | ||
| if (material.value("type", std::string{}) == "props_scalar") |
There was a problem hiding this comment.
Note, not a finding — the JSON layer now hardcodes the factory type name "props_scalar" to decide the binding time.
I checked the failure mode if that name ever drifts: the branch falls through to value substitution, which sets a "value" key on a material that declares no such parameter and requires "index" — so construction fails with a missing-required-parameter error rather than silently mis-binding. Loud, which is acceptable.
Leaving it, but flagging that this is the one string coupling between the JSON layer and a concrete material type.
There was a problem hiding this comment.
Left as-is, as flagged. The drift failure mode is loud (missing required index), and the alternative — a virtual on material_interface or a factory-side capability flag — is more machinery than one string comparison warrants right now.
Worth revisiting if a second material ever needs per-call host data, since then the coupling stops being a special case.
…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.
|
All three findings fixed in 751f6f5. 211 tests, up from 207.
Every new test was verified to fail against the code it guards — the coarse guard, the disabled bound-check, and a stubbed-out forward respectively. A test for a silent-corruption bug that passes either way is worse than none, and finding 1 was exactly that kind of bug: wrong, plausible, no diagnostic. Finding 1 is worth noting as a process point. Every test in the original PR was all-baked or all-live, so nothing could have caught it. The guard was correct for the case I had in mind and silently absent for the one I didn't — the same shape as the |
…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.
…tants # Conflicts: # include/numsim-materials/umat/umat_interface.h
…ng times Merging the json_model fixes exposed the duplication they warned about. The declared-parameter check validated bindings[i].property while the substitution loop wrote "index" for a props_scalar -- so every props_scalar document was rejected at registration with "props_scalar does not declare 'value'", which is true of the parameter and false of the target. bound_parameter() now answers both questions in one place: which parameter is written, and whether the value is the host constant or the slot it lives in. Validation and substitution call it, so they cannot drift. The target still names the graph property the constant arrives on -- "value", the same as constant_scalar -- because keeping the spelling identical across the two binding times is what lets the type be swapped without rewriting "constants". Naming "index" instead is rejected, since the slot comes from the entry's position and a target that appeared to set it would be misleading.
# Conflicts: # include/numsim-materials/umat/json_model.h
# Conflicts: # include/numsim-materials/umat/json_model.h
petlenz
left a comment
petlenz
left a comment
There was a problem hiding this comment.
Second review, after the fixes. One finding, and it is one I half-fixed last round.
Also re-verified that the earlier fixes still hold: the mixed-document probe reports a fatal and zeroed outputs, the clobber test still fails against a stored-pointer build, and the per-slot guard still fails when reverted to per-model.
| auto& ts = thread_state_for(cmname, props); | ||
| if (c.ec == element_case::plane_stress) | ||
| if (c.ec == element_case::plane_stress) { | ||
| ts.ps->bind_props(props); |
There was a problem hiding this comment.
Untested, and it is the production plane-stress path.
Deleting this line passes 214/214. Last review I added PropsScalar.PlaneStressUsesTheLiveConstants, but that drives plane_stress_evaluator directly — so I covered the forward inside the evaluator and left the registry dispatch that calls it uncovered. The half I fixed was the half nothing was actually going to break.
The severity is limited by the guard added in the same round. With the line removed, a plane-stress call through umat_ gives:
FATAL: material_point_evaluator: this model reads its material constants
per call — call bind_props() before evaluating
C1111=0.0000 fatals=1
So it aborts rather than running with moduli of zero. That is the never-bound check earning its keep on a path I had not considered when I added it — which is the argument for that kind of guard over a comment saying "remember to bind".
Still worth closing: one test driving plane stress (NDI=2, NSHR=1) with live constants through the real entry point. I have the probe already; it returns C1111=118.2609 against the expected E/(1-v^2)=118.2609.
There was a problem hiding this comment.
Fixed. PropsScalarJson.PlaneStressBindsThroughTheRegistry drives NDI=2, NSHR=1 through umat_ and asserts the condensed E/(1-v^2) derived from the constants the host supplied.
Verified load-bearing: with ts.ps->bind_props(props) removed the test aborts on the never-bound guard rather than passing. 216 tests.
# Conflicts: # include/numsim-materials/umat/json_model.h
PropsScalar.PlaneStressUsesTheLiveConstants drives plane_stress_evaluator directly, so it covered the forward inside the evaluator and left the registry dispatch that calls it uncovered -- deleting ts.ps->bind_props(props) passed the whole suite. The half that was fixed was the half nothing was going to break. Now driven through umat_ with NDI=2, NSHR=1, asserting the condensed E/(1-v^2) derived from the constants the host supplied. With the registry line removed it aborts on the never-bound guard, so the test is load-bearing either way.
1 participant
Stacked on #30, and merges #27 — see "Why it needs #27" below.
constant_scalarcopies its number into the graph at construction. That is right for Abaqus, where PROPS is fixed for a material name and the registry enforces it. It is wrong for CalculiX, which interpolates the*USER MATERIALconstants by temperature so they can differ from one call to the next.props_scalarrecords only which slot it owns and takes the number from the host each call.Rebuilding the graph per call would also be correct — nothing is retained between calls — but it measures 22x an evaluation even from a hand-written builder (6.7 µs against 302 ns), where reading a slot is free.
Usage — one word changes
"materials": [ {"type": "external_strain_source", "name": "strain_in"}, - {"type": "constant_scalar", "name": "K", "value": 0}, + {"type": "props_scalar", "name": "K"}, {"type": "isotropic_tangent", "name": "stiffness", "K_source": "K", ...}, ... ], "constants": ["K::value", "G::value"]The position in
"constants"is the slot, so the binding is written exactly as before. No"index"in the document, nothing downstream notices.bind()dereferences and keeps nothingThis is the part worth reviewing. A host constants array may be a per-call temporary, so an implementation that stored
const double*would read a dead stack slot on the next call — and usually return the right number anyway, because the slot is commonly reused. That failure survives every ordinary test.PropsScalar.ReadsTheConstantsAtBindTimeNotAtUpdateTimeclobbers the caller's buffer betweenbind_propsandevaluate. Verified it fails against a stored-pointer implementation.Two properties inherited from
constant_scalarPlain property, no update callback. So nothing reaches
statev_map— a modulus costs no STATEV slot, unlikeexternal_scalar_sourcewhich would cost one each — and the value is in place beforectx.update(), so graph ordering cannot affect it.Readers are collected, not configured
The evaluator finds them with the same
dynamic_castprobestatev_mapalready uses. A hand-listed set inconfigwould be a second thing to keep in step with the graph, and a missed entry is a silently stale modulus.Why it needs #27
The registry's PROPS-consistency check must stand down for these models: a changed constants array is the point, not a contradiction, because nothing was baked into the graph for it to contradict.
I first based this on #30 alone and found the guard was untestable there — #30's base only compares NPROPS counts, and the test supplies two constants either way, so it passed with the guard removed. Merging #27 makes the value comparison exist; the guard is now load-bearing and verified to fail without it.
Tests
umat_entry point via JSON — including that the same graph gives two different stiffnesses for two different constant sets with no rebuild, and back again.207 total on the branch.