materials: let weighted_sum and isotropic_damage source a tangent separately - #29
materials: let weighted_sum and isotropic_damage source a tangent separately#29petlenz wants to merge 12 commits into
Conversation
…arately Both assumed the material producing a constituent's stress also produces its tangent. That holds for linear_elasticity and stops holding as soon as the stiffness is its own material, so both gain an optional override. weighted_sum takes tangent_sources, one entry per term. Positional matching is a trap on its own: a shorter list applies each override to the WRONG constituent, both names resolve, wire_inputs() succeeds, and the only symptom is a wrong summed tangent -- degraded Newton convergence while the stresses still converge correctly. So the length must match the term count exactly, and "" keeps a term's own tangent, which is also what lets a non-leading term be overridden alone. isotropic_damage takes tangent_source, declared optional and checked with contains() at the use site rather than defaulted to "", so unset and deliberately-empty stay distinguishable. weighted_sum had no test file at all; it has five now, covering the mixture rule itself and every branch of tangent_sources including the length check.
petlenz
left a comment
petlenz
left a comment
There was a problem hiding this comment.
Critical review — probed rather than read. No correctness defects found. One dead-code trap and one limitation worth documenting.
What I checked and could not break:
- The
"" is an error, not a fallbackclaim holds. An emptytangent_sourceresolves as a material name of"", which fails at wire time like any unknown name. So unset and deliberately-empty really do stay distinguishable, as the comment says. - The length check covers both directions, and the
""escape does let a non-leading term be overridden alone — that combination is what makes positional matching safe here. isotropic_damage'scontains()at the use site is right: aset_defaultof""would have collapsed the two states.
Two notes inline.
Dead code: weighted_sum::update() (line 95, outside the diff)
This override is never called. property_engine::update() dispatches per property — prop->traits().update() — and add_output("stress", &weighted_sum::update_stress) installs those callbacks directly. Nothing invokes the material's virtual update(); only backward_euler and vector_newton route it deliberately, by assigning traits().update = [this]{ this->update(); }.
So today it is harmless duplication. The trap is directional: someone adding a third output and wiring it only into update() — the function that looks like the entry point — gets a property that silently never recomputes. That is the same failure mode as the intra-material ordering hazard this stack exists to fix, arrived at from the other side.
Pre-existing, not introduced here, but this PR is the one touching the file. I would delete it, or keep it and say in a comment that it exists for a solver-driven context and is not the engine's entry point.
| // the wrong constituent — both names resolve and the only symptom is a | ||
| // wrong summed tangent. Require one entry per term, and let "" mean "this | ||
| // term keeps its own", so a non-leading term can be overridden alone. | ||
| if (!m_tangent_sources.empty() && |
There was a problem hiding this comment.
Limitation worth stating in the doc comment rather than a defect.
The length check closes the case where a short list shifts every override onto the wrong constituent. It cannot close the case where a correct-length list names the wrong material — say tangent_sources = {"", "matA_stiff"} when term 1 is matB. Both names resolve, wire_inputs() succeeds, and the symptom is identical to the bug this check was added for: correct stresses, a wrong summed tangent, degraded Newton convergence and nothing in any log.
That is inherent to decoupling stress from tangent — the whole point of the parameter — so no validation can catch it. But the class comment currently reads as though supplying one entry per term is sufficient for correctness, and it is only sufficient for alignment. One sentence saying the entry must name the material that produced that term's stiffness would earn its place.
There was a problem hiding this comment.
Both done, no behaviour change.
The dead update() is deleted, with a comment in its place saying why there isn't one — that the engine drives each output through its add_output callback, and that backward_euler/vector_newton only use update() because they route it themselves. Better than silence: the absence is now deliberate rather than looking like an oversight.
The class comment now states the limitation outright — one entry per term buys alignment, not correctness, and no validation can close the wrong-name case because decoupling stress from tangent is the point of the parameter.
…sources cannot check Two review notes, no behaviour change. The update() override was never called. property_engine drives each output through the callback registered with add_output, so nothing invokes a material's virtual update() unless the material routes it itself, as backward_euler and vector_newton do. Harmless duplication today, and a trap in one direction: an output wired only into update() -- the function that looks like the entry point -- would silently never recompute. The class comment read as though one entry per term were sufficient for correctness. It is sufficient for ALIGNMENT. A correct-length list naming the wrong material wires, runs, and produces the same symptom the length check was added to prevent: correct stresses, a wrong summed tangent, degraded Newton convergence and nothing in any log. No validation can catch it, since decoupling stress from tangent is the point of the parameter -- so it is said outright instead.
…t-source-consumers
|
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). |
…t-source-consumers
…t-source-consumers
…t-source-consumers
…t-source-consumers
…t-source-consumers
…t-source-consumers
…t-source-consumers
…t-source-consumers
1 participant
Stacked on #28.
Both materials assumed the material producing a constituent's stress also produces its tangent. That holds for
linear_elasticityand stops holding the moment the stiffness is its own material (#28), so both gain an optional override.weighted_sum:tangent_sourcesOne entry per term. Positional matching is a trap on its own — a shorter list applies each override to the wrong constituent: both names resolve,
wire_inputs()succeeds, and the only symptom is a wrong summed tangent, i.e. degraded Newton convergence while the stresses still converge correctly. Nothing in a log would show it.So the length must match the term count exactly, and
""keeps a term's own tangent — which is also what lets a non-leading term be overridden alone.isotropic_damage:tangent_sourceDeclared optional and checked with
contains()at the use site rather than defaulted to"", so unset and deliberately-empty stay distinguishable.Tests
weighted_sumhad no test file at all. It has five now: the mixture rule itself, absenttangent_sources, an override, the empty-entry escape, and the length check in both directions.Split out of #26.