Enable coupled dof equation condition in 3d#1751
Enable coupled dof equation condition in 3d#1751m-frey wants to merge 1 commit into4C-multiphysics:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Enables PointLinearCoupledEquation constraints for 3D displacement coupling by introducing support for the dispz degree of freedom in both input validation and MPC assembly.
Changes:
- Allow
ADD: "dispz"in theDESIGN POINT COUPLED DOF EQUATION CONDITIONSinput schema. - Map
dispzto the third displacement DOF index when building linear MPC equations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp |
Adds dispz DOF mapping for linear coupled equation MPC construction. |
src/constraint_framework/4C_constraint_framework_input.cpp |
Extends the allowed ADD selection list to include dispz. |
Comments suppressed due to low confidence (1)
src/constraint_framework/4C_constraint_framework_submodelevaluator_mpc.cpp:643
- The exception message still says linear coupled equations are only implemented for 2D (
dispx/dispy), butdispzis now accepted above. Update the error text to reflect the actual supported DOFs and (ideally) mention thatdispzrequires a 3D problem/RVE.
FOUR_C_THROW(
"Linear coupled equations (MPCs) are only implemented for 2D (dispx or dispy DOFs)");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (dofStr == "dispz") | ||
| { | ||
| dofPos = 2; | ||
| } |
There was a problem hiding this comment.
dispz is now mapped to dofPos = 2, but there is no guard that the current problem/RVE actually has a third displacement DOF. In a 2D setup, this will likely index discret_ptr_->dof(node)[2] out of range. Add a dimension/DOF-count check (e.g., based on rve_dim_ or discret_ptr_->dof(node).size()) and throw a clear error when dispz is used in 2D.
| Core::Conditions::PointLinearCoupledEquation, false, Core::Conditions::geometry_type_point); | ||
|
|
||
| linear_ce.add_component(parameter<int>("EQUATION", {.description = "EQUATION"})); | ||
| linear_ce.add_component(deprecated_selection<std::string>("ADD", {"dispx", "dispy", "undefined"}, | ||
| {.description = "degrees of freedom", .default_value = "undefined"})); | ||
| linear_ce.add_component( | ||
| deprecated_selection<std::string>("ADD", {"dispx", "dispy", "dispz", "undefined"}, |
There was a problem hiding this comment.
The condition definition text right above still describes the coupled DOF equation as being for "2d", but the allowed DOFs now include dispz (3D). Please update the human-readable description so the input schema matches the actual capabilities and doesn't mislead users.
| linear_ce.add_component( | ||
| deprecated_selection<std::string>("ADD", {"dispx", "dispy", "dispz", "undefined"}, | ||
| {.description = "degrees of freedom", .default_value = "undefined"})); |
There was a problem hiding this comment.
Adding dispz changes supported input behavior, but there is no regression test covering a 3D DESIGN POINT COUPLED DOF EQUATION CONDITIONS term with ADD: "dispz". Consider adding a 3D integration test input (similar to tests/input_files/rve3d_periodic_bcs.4C.yaml, but including a PointLinearCoupledEquation section with dispz) and registering it in tests/list_of_tests.cmake (there is already a 2D MPC test: rve2d_periodic_bcs_with_mpcs.4C.yaml).
isteinbrecher
left a comment
There was a problem hiding this comment.
Looks good to me, can you add a small test case for this feature?
maxfirmbach
left a comment
There was a problem hiding this comment.
@m-frey I just have a small remark. I would also like to see a test that explicitly uses the new feature e.g. ADD: "dispz" somewhere.
| else | ||
| { | ||
| FOUR_C_THROW( | ||
| "Linear coupled equations (MPCs) are only implemented for 2D (dispx or dispy DOFs)"); |
There was a problem hiding this comment.
I think this error message also has to be adapted right?
ischeider
left a comment
There was a problem hiding this comment.
Just asking for some more documentation ;-)
| @@ -110,8 +110,9 @@ | |||
| Core::Conditions::PointLinearCoupledEquation, false, Core::Conditions::geometry_type_point); | |||
|
|
|||
| linear_ce.add_component(parameter<int>("EQUATION", {.description = "EQUATION"})); | |||
There was a problem hiding this comment.
Could you please also change the description of EQUATION to something more useful? What kind of equation is possible, with which dependencies? See also comment to COEFFICIENT.
If it refers to a function, then shouldn't we redefine the parameter to FUNCT, which is the name that we use in general? Even though it is a breaking change then.
| linear_ce.add_component( | ||
| deprecated_selection<std::string>("ADD", {"dispx", "dispy", "dispz", "undefined"}, | ||
| {.description = "degrees of freedom", .default_value = "undefined"})); | ||
| linear_ce.add_component(parameter<double>("COEFFICIENT")); |
There was a problem hiding this comment.
The coefficient does not have a description at all. Since there is also EQUATION available, I don't know what an additional coefficient could do (maybe I fully misunderstand the parameters). If it is a factor added to the function/equation, I recommend to make it optional with 1 as a default value.
| /*----------------------------------------------------------------------*/ | ||
| Core::Conditions::ConditionDefinition linear_ce("DESIGN POINT COUPLED DOF EQUATION CONDITIONS", | ||
| "PointLinearCoupledEquation", | ||
| "definition of the term of a linear couple equation coupling different degrees of " |
There was a problem hiding this comment.
| "Definition of the term of a linear couple equation coupling specified degrees of " |
isteinbrecher
left a comment
There was a problem hiding this comment.
Can we add a test case for this feature?
Description and Context
This PR enables PointLinearCoupledEquation constraints for 3D displacement coupling by adding support for the dispz degree of freedom. There are no additional test as the underlying functionality is already tested in 3d.