Add tests, docs, and generalization to #79 - #84
Merged
Conversation
…ndling Builds on PR KnutAM#79 (JG/set_new_to_old_state) from GunnarssonJ, which adds set_new_to_old_states! to reset the current (new) state variables back to the last converged (old) state without reassembling. While writing tests, found that the PR's implementation used copyto! unconditionally, which throws a MethodError for any per-cell state that isn't an AbstractArray (e.g. a single mutable struct per cell). Fixed src/states.jl to fall back to a deepcopy-based assignment for such states, keeping the zero-allocation copyto! path for the common Vector{T} case. Added a docstring for set_new_to_old_states! (mirroring update_states!), exported it from FerriteAssembly.jl, and documented it in docs/src/DomainBuffers/StateVariables.md and Setup.md. Added tests in test/states.jl covering: bits-vector states (MatA), non-bitstype mutable-struct-per-cell states (MatB, exercises the deepcopy fallback), accumulating states (MatC), nothing-states, Simulation forwarding, and Dict{String}-domain forwarding. Tests check correctness, non-aliasing with old_states, and allocation behavior. Test results: Pkg.test() passes (335/335 state-variable tests, full suite green). docs/make.jl (which executes all tutorials/howtos via Literate and builds the full doc site) completes with no errors.
set_new_to_old_states! previously used a hardcoded deepcopy fallback for per-cell states that aren't AbstractArrays (e.g. a single mutable struct per cell). Introduce FerriteAssembly.copy_state(state), defaulting to deepcopy, as the dispatch point for that fallback so users can overload it for a custom cell state type and avoid the allocation. AbstractArray states are unaffected and continue to use the zero-allocation copyto! path. Documented copy_state in the set_new_to_old_states! docstring and in docs/src/DomainBuffers/StateVariables.md (added to the API list). Added a MatD/StateD test case in test/states.jl (mutable struct per cell, like MatB) that overloads copy_state and uses a call counter to confirm the custom method is dispatched for every cell during set_new_to_old_states!, while values and non-aliasing with old_states remain correct. MatB (no overload) continues to test the default deepcopy fallback. Test results: Pkg.test() "state variables" testset passes cleanly at 341/341 (up from 335) across repeated runs, with all other testsets matching a previously fully-completed clean run (the test tool has a ~170s cap that truncates the last few testsets' output, unrelated to these changes). docs/make.jl (executes every tutorial/howto and builds the full doc site) completes with no errors.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 96.82% 96.89% +0.06%
==========================================
Files 30 30
Lines 1166 1190 +24
==========================================
+ Hits 1129 1153 +24
Misses 37 37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… in the AbstractArray cell state case
The copy_state/set_new_to_old_states! interface in src/states.jl was
redesigned: copy_state no longer has a default deepcopy fallback (it's
now `function copy_state end`), isbits values are copied by identity
internally via _copy_state, and AbstractArray states are copied
element-wise via map! instead of copyto!, dispatching copy_state per
non-isbits element.
Updated test/states.jl to match:
- Added FerriteAssembly.copy_state(s::StateB) = deepcopy(s): StateB
previously relied on the removed default fallback and would now throw
a MethodError.
- Added MatE/StateE, a Vector{StateE} of non-bits elements, to exercise
the per-element copy_state dispatch inside map! (with a call counter
confirming it's invoked once per array element, and a non-aliasing
check).
- Added MatF/StateF with no copy_state overload at all, asserting
set_new_to_old_states! throws MethodError, to directly test that the
old deepcopy fallback is gone.
- Updated stale comments on MatB/MatD referring to the old fallback.
Updated docstrings in src/states.jl (copy_state), src/DomainBuffers.jl
and docs/src/DomainBuffers/StateVariables.md (set_new_to_old_states!) to
accurately describe: per-value (not per-declared-type) isbits identity
copying, the mandatory copy_state overload with no default, and that
AbstractArray states must be mutable and keep matching axes between
old/new since they're updated in-place via map!.
Reviewed via dual-review with Codex (independent read-only reviewer) at
both the plan and diff stage; addressed all findings (docstring
precision issues, missing no-fallback regression test, missing array
mutability note) except the map! axes/mutability requirement itself,
which is now documented as an invariant rather than enforced in code,
since the underlying set_new_to_old_states! implementation was the
user's own redesign and out of scope for this change.
Test results: Pkg.test() "state variables" testset passes 360/360 (up
from 341). docs/make.jl builds cleanly (fixed an unrelated @ref link
break introduced while editing StateVariables.md).
set_new_to_old_states! now additionally checks ismutable(old_val) before taking the element-wise map! path: only a mutable AbstractArray cell state is updated element-wise; anything else, including an immutable AbstractArray (e.g. NTuple- or StaticArrays-backed), is now copied as a whole via copy_state, same as any other non-array cell state. It also now throws ArgumentError on an axes mismatch between the old and new mutable-array states, instead of silently truncating via map!. Updated docstrings in src/states.jl, src/DomainBuffers.jl, and docs/src/DomainBuffers/StateVariables.md to describe this: mutable vs. immutable AbstractArray handling, and the new ArgumentError on axes mismatch. Added test/states.jl coverage: - MatG/StateG/ImmutableStates: an immutable AbstractVector wrapping mutable-but-non-isbits StateG elements, with a copy_state overload for the whole array. Confirms the whole-array path is taken (copy_state called once per cell, not once per element) and that values are copied correctly without aliasing. - MatH/ImmutableStatesNoOverload: same immutable-array shape but with no copy_state overload, confirming MethodError (not a map!-related error) for an immutable array cell state without an overload. - A regression test using MatA with an artificially resized "new" state vector, confirming set_new_to_old_states! now throws ArgumentError on a mutable-array axes mismatch. Test results: Pkg.test() "state variables" testset passes 368/368 (up from 360). docs/make.jl builds cleanly.
`update_states!` swaps the old and new state containers, so directly after it the
new states hold the values from the previous time step. The phase-field fracture
tutorial couples its two parts through the states: the `:u` part reads the *new*
phase-field state via `get_state(cb_d)`, and it is assembled first in the staggered
loop. Without a copy back, the first displacement solve of each time step therefore
used a phase field that was one time step too old.
Added `set_new_to_old_states!(sim_d)` after `update_states!(sim_d)`, plus an inline
comment and a new section explaining the swap-vs-copy semantics, that the old states
(the irreversibility bound) are unaffected, and that no `copy_state` overload is
needed for the `Vector{Float64}` cell state. The copy also prevents the
irreversibility bound from moving backwards in the case where the `:d` part is never
assembled during a time step.
Verified by running the tutorial's solve with and without the call, with fresh
buffers, simulations, matrices and constraint handler per variant (65 load steps,
threading, autodiffbuffer):
* sent_fine.inp (the tutorial's grid): staggered iterations 713 -> 687, Newton
iterations 2033 -> 1922, wall time 472.8 s -> 404.5 s. Same solution:
max|dd| = 6.7e-6 (max|d| = 0.91), max|dphi_old| = 8.8e-6, reaction-force curve
relative difference 1.3e-5.
* sent_coarse.inp: staggered 618 -> 588, Newton 1700 -> 1578, wall 58.4 s -> 47.3 s,
max|dd| = 1.5e-5, reaction-force curve relative difference 4.2e-6.
Test results: Pkg.test() passes (all testsets, "state variables" 368/368). Every
literate tutorial and how-to script runs. docs/make.jl builds with no cross-reference
errors (only the pre-existing viscoelasticity example_size_threshold warning).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WazatEo5GFZNfTZ4gwRLfj
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
2 participants
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.
Add tests, docs, and generalization to #79