Fix monitor_dbc DOF iteration in 2D#2036
Conversation
Solid::MonitorDbc looped DOF-indexed access up to the hard-coded DIM = 3 in three functions. In 2D problems this caused: - get_reaction_moment: read a non-existent third DOF, feeding uninitialised memory into node_position[2] and the cross_product that builds the reaction moment. The result was incorrect m_x / m_y or an FPE overflow during the structural predictor. - create_reaction_maps: read onoff[2] out-of-bounds. Latent undefined behaviour that did not crash deterministically because the heap region after the 2-element vector often happens to hold zero. - get_reaction_force: iterated d = 2 over an empty react_maps[2]. No crash, but inconsistent with the n_dim based logic of the other two functions. Replace the loop bounds with discret.n_dim(). The 3D storage matrices stay 3-component because cross_product is 3D-only; the out-of-plane components are then exactly zero in 2D, which is physically correct (m_z = x*F_y - y*F_x is the only non-trivial 2D moment). Also add a defensive node_position.put_scalar(0.0) before the position fill loop in get_reaction_moment. Cover the bug with a 2D regression test: a 2x1 plane-strain QUAD4 mesh with a tagged LINE Dirichlet on the left edge and a y-dependent x-pull on the right edge. The reference CSV pins f_z, m_x and m_y to exactly zero under TOL_A = 1e-12, so any regression in DIM-vs-n_dim handling surfaces immediately. Fixes 4C-multiphysics#2032.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes out-of-bounds DOF iteration in Solid::MonitorDbc when running 2D problems. The loops previously used a hard-coded 3D extent, reading uninitialised memory that occasionally caused FPE overflows in the moment computation.
Changes:
- Replace
DIM-bound loops with the discretisation's actualn_dim()in three functions ofmonitor_dbc. - Zero-initialise
node_positionper iteration so the out-of-plane component is exactly zero in 2D. - Add a 2D regression test (input file, reference CSV, CMake registration).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/structure_new/src/utils/4C_structure_new_monitor_dbc.cpp | Use n_dim for DOF-indexed loops in create_reaction_maps, get_reaction_force, and get_reaction_moment; clear node_position per node. |
| tests/input_files/solid_monitor_dbc_2d.4C.yaml | New 2D plane-strain input pinning the previously corrupted moment columns. |
| tests/input_files/ref/solid_monitor_dbc_2d_1001.csv | Reference CSV for the new test. |
| tests/list_of_tests.cmake | Register the new 2D monitor_dbc test with CSV comparison. |
| // empty (valid) map so downstream 3D-iterating code keeps working. | ||
| const unsigned n_dim = discret.n_dim(); | ||
|
|
||
| std::vector<int> my_dofs[DIM]; |
| std::vector<double> mydisp = Core::FE::extract_values(*dispn, node_gid); | ||
| for (unsigned i = 0; i < DIM; ++i) node_position(i) = node->x()[i] + mydisp[i]; | ||
| node_position.put_scalar(0.0); | ||
| for (unsigned i = 0; i < n_dim; ++i) node_position(i) = node->x()[i] + mydisp[i]; |
|
I think the current state introduces a bit of ambiguity. Now we have Edit: This would also avoid comments in the actual implementation, which should be self explaining. |
| @@ -0,0 +1,78 @@ | |||
| TITLE: | |||
| - "Regression test for the monitor_dbc moment-loop bug in 2D problems." | |||
There was a problem hiding this comment.
I honestly don't like this description that much. It describes a bug, which should be fixed with this PR, so no need to elaborate on it here. In addition, the description is quite detailed referring to the actual implementation and is again past tense ... so referring to something that has been like that. We should add information what is actually tested and not what bug is triggered (because it isn't).
maxfirmbach
added
type: enhancement
|
@bennoschoenstein Closes #2032. Right? |
|
@maxfirmbach thank you for your comments, and you are right, it would be better to be able to solve the dimensional problem beforehand. I will think of a way how to solve this problem more elegantly and reduce the unnecessary long comments. And yes, it would also solve/close Issue #2032 once its merged |
Problem
In 2D the code reads past the end of two-element arrays. Most of the time the garbage that comes back is silently written into the moment columns of the CSV. Occasionally it is large enough to overflow the cross product that builds the moment, aborting the simulation. In practice this caused random crashes in my 2D RVE convergence studies.
Fix
The DOF-indexed loops now use the actual spatial dimension of the discretisation. The 3-component storage matrices stay as they are because the cross product is only defined in 3D — the out-of-plane components then come out exactly zero in 2D, which is the physically correct answer (
m_z = x·F_y − y·F_x).A code review surfaced two more functions with the same pattern; all three are fixed here.
Validation
solid_monitor_dbcregression tests still pass.under
TOL_A = 1e-12.Fixes #2032.
Vorher:
Nachher:
