core: write a material parameter in place, without invalidating bound references - #47
Merged
Merged
Conversation
… references
Materials bind their parameters once, in the constructor:
m_K(base::template get_parameter<value_type>("K"))
and hold the result as a `const value_type&`. That reference points at the
object inside the std::any inside the handler's map node, so a parameter can be
updated after construction and every material observes it — but only if the
write leaves the object where it is.
template <typename T>
void set_parameter(std::string const& key, T const& value) {
m_parameter_handler.template get<T>(key) = value;
}
Assigning through the NON-CONST get<T>() is the whole point, and insert() is
deliberately not used. insert() goes through insert_or_assign, which replaces
the entire std::any; for a value larger than std::any's small buffer that
destroys the contained object and constructs a new one elsewhere, dangling every
reference a material bound at construction.
The distinction is invisible with scalar parameters, which is what makes it
worth a test rather than a comment: a `double` fits the small buffer, so
insert() happens to preserve its address and an implementation built on insert()
would pass any test written against moduli — then break the first time someone
stored a tensor-valued parameter. test_set_parameter asserts the address is
stable for a 256-byte parameter, and separately pins the underlying
parameter_handler behaviour (insert relocates, assignment does not) so the
rationale lives next to the mechanism rather than only in this message.
This does nothing about quantities DERIVED from parameters. A material that
precomputes something in its constructor will not notice a later write; that is
a per-material concern and is handled where the derived value lives.
Motivation is host-driven material constants: Abaqus fixes PROPS per material
name, but CalculiX interpolates *USER MATERIAL constants by temperature, so they
genuinely vary between calls and the graph has to be able to follow them.
Kept the two facts a reader cannot recover from the code — insert_or_assign relocates anything past std::any's small buffer, and derived values are not invalidated — and cut the explanation around them.
…othing
Six of the ten review findings on this PR. Two are fixed in the API, one is
guarded, and the rest are limitations now pinned by tests against real materials
rather than left for a user to discover.
T is no longer deduced. std::type_identity_t makes the stored type an explicit
argument, so set_parameter<double>("K", 250) is fine while set_parameter("K",
250) fails to COMPILE. Previously it deduced int, threw std::bad_any_cast, and
lost the write — and bad_any_cast derives from neither invalid_argument nor
runtime_error, so a UMAT boundary catching those would miss it and terminate.
A remaining mismatch (set_parameter<float> against a stored double) is now
translated into an invalid_argument naming the key, instead of a bare
"bad any_cast" naming nothing.
Writing "name" is rejected. The identity is cached in m_name at construction and
used as the material_handler registry key, so a write left the parameter
disagreeing with both: name() and lookups kept the old value while
get_parameter<std::string>("name") reported one that resolved to nothing.
The remaining findings are real but not fixable at this level, so they are
documented precisely and pinned by tests. A write reaches only what the material
re-reads through its bound reference. It does not affect anything derived at
construction, copied into a member, or consumed once for wiring; and it is local
to one material, because each holds its own copy of the handler. The doc comment
previously claimed more than that, and pointed at isotropic_tangent's "recompute"
as the mitigation — a class that does not exist on this branch.
The test gap was the reason all of this shipped. Every case went through a probe
material that binds each parameter by reference and derives nothing, which is
precisely the shape the API handles cleanly. Three new tests use SHIPPED
materials and pin the awkward truth instead:
WriteLandsButDerivedStateGoesStale linear_elasticity: K changes, stress does
not, because the tangent was built once
WriteIsLocalToTheMaterial the caller's handler keeps the old value
WritingAWiringKeyDoesNotRewire a source name written after finalize()
leaves the input wired as before
Plus guards for the two fixes, both mutation-verified: removing the name check
and breaking the bad_any_cast translation each fail exactly their own test.
Not addressed: parameter_handler::insert remains public and still relocates, so
set_parameter only avoids the hazard for callers who go through it. And the
underlying gap — no way to invalidate anything derived from a parameter — is a
framework-level design question, currently answered per-material by
isotropic_tangent's "recompute" flag on the child branch.
184 -> 190 tests.
# Conflicts: # tests/CMakeLists.txt
Same four facts -- in-place assignment through get<T>() rather than insert(), why T is not deduced, and what a write does not reach -- with the prose and the parenthetical examples cut. 15 lines to 12.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
material_interface::set_parameter<T>(), so a caller can change a material's parameter after construction and have the material see it — a UMAT re-readingPROPSbetween increments, a driver sweeping a modulus, a test varying one input without rebuilding the graph.The branch predates the plasticity split and the UMAT layer;
mainis merged in here. The only collision wastests/CMakeLists.txt(both sides append a test registration). 274/274 tests pass, verified locally against the merge result.What it does
Assigns through the non-const
get<T>(), notinsert().insert_or_assignreplaces the wholestd::any, which relocates anything past its small-buffer — invalidating every reference a material bound at construction, which is how every material reads its parameters.Tis deliberately not deduced.std::type_identity_tmakes the stored type an explicit argument, soset_parameter<double>("K", 250)compiles andset_parameter("K", 250)does not. Deduced, that literal was anint, threwstd::bad_any_cast, and silently lost the write — andbad_any_castderives from neitherinvalid_argumentnorruntime_error, so a UMAT boundary catching those would miss it entirely and terminate. A surviving mismatch (set_parameter<float>against a storeddouble) is translated into aninvalid_argumentnaming the key.Writing
"name"is rejected: the identity is cached inm_nameat construction and used as thematerial_handlerregistry key, so a write left the parameter disagreeing with both —name()and lookups kept the old value whileget_parameter<std::string>("name")reported one that resolved to nothing.What it deliberately does not do
A write reaches only what the material re-reads through its bound reference. It does not touch anything derived at construction, copied into a member, or consumed once for wiring, and it is local to one material because each holds its own copy of the handler.
That is a real limitation, and the reason it is stated so precisely is that the original test suite hid it: every case went through a probe material that binds each parameter by reference and derives nothing — exactly the shape the API handles cleanly. Three tests now use shipped materials and pin the awkward behaviour instead:
WriteLandsButDerivedStateGoesStalelinear_elasticity:Kchanges, stress does notWriteIsLocalToTheMaterialWritingAWiringKeyDoesNotRewirefinalize()leaves the input wired as beforeBoth fixes are mutation-verified: removing the
"name"check and breaking thebad_any_casttranslation each fail exactly their own test.One note for review
The doc comment's original wording pointed at a recompute-style tangent as the mitigation for the staleness above, and noted no such class existed. One does now —
isotropic_tangent(#28) rebuilds on every update with no memo. It sourcesK/Gfrom other materials rather than from its own parameters, so it is not directly theset_parameterpath, but it may be the better answer for a caller who wants a live modulus, and the comment could say so.