-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GeoMechanicsApplication] Initialize all entities (regardless of active/inactive status) in the geo scheme #13152
[GeoMechanicsApplication] Initialize all entities (regardless of active/inactive status) in the geo scheme #13152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet another step towards being able to simulate the quay wall model using the build from master
. Thank you for the clear fix and the well-documented test. I have a few questions and suggestions. Hope this helps.
@@ -53,8 +53,10 @@ class GeoMechanicsSchemeTester | |||
mModel.CreateModelPart("dummy", 2); | |||
} | |||
|
|||
template <class T> | |||
void TestFunctionCalledOnComponent_IsCalledOnActiveAndInactiveComponents() | |||
template <class T, typename AddComponentToModelPartLambda> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can make this type name slightly more general? For instance:
template <class T, typename AddComponentToModelPartLambda> | |
template <typename T, typename AddComponentToModelPartCallable> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
KRATOS_TEST_CASE_IN_SUITE(FunctionCalledOnElement_IsCalledOnActiveAndInactiveElements, KratosGeoMechanicsFastSuiteWithoutKernel) | ||
KRATOS_TEST_CASE_IN_SUITE(FunctionCalledOnElement_IsCalledOnActiveAndInactiveConditions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Conditions" should be "Elements":
KRATOS_TEST_CASE_IN_SUITE(FunctionCalledOnElement_IsCalledOnActiveAndInactiveConditions, | |
KRATOS_TEST_CASE_IN_SUITE(FunctionCalledOnElement_IsCalledOnActiveAndInactiveElements, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"K0_MAIN_DIRECTION" : 1, | ||
"K0_NC" : 0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the K0 procedure parameters are redundant here, since we don't apply the K0 procedure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, removed 👍
"value": [10000,0.0] | ||
} | ||
}], | ||
"list_other_processes" : [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of this empty list.
"value" : [0.0,0.0,0.0], | ||
"table" : [0,0,0] | ||
} | ||
},{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why in the second stage the horizontal displacements of all nodes are no longer constrained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is an oversight from my side, corrected it now, good catch!
"reduction_factor" : 0.5, | ||
"increase_factor" : 2.0, | ||
"desired_iterations" : 4, | ||
"calculate_reactions" : true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In stage 1, this flag was set to false
, now it is true
. Can you help me to understand why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I wanted to calculate as little as possible so put it to false in stage 1, but forgot stage 2. Made it more consistent now
|
||
file_pattern = os.path.join(project_path, "ProjectParameters_stage*.json") | ||
stage_files = glob.glob(file_pattern) | ||
print(stage_files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be intended for debugging. I'd suggest to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
…emoving SQ code smells
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Processed the review comments, fixed SQ issues and did some formatting on the python file
"K0_MAIN_DIRECTION" : 1, | ||
"K0_NC" : 0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, removed 👍
"reduction_factor" : 0.5, | ||
"increase_factor" : 2.0, | ||
"desired_iterations" : 4, | ||
"calculate_reactions" : true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I wanted to calculate as little as possible so put it to false in stage 1, but forgot stage 2. Made it more consistent now
"value" : [0.0,0.0,0.0], | ||
"table" : [0,0,0] | ||
} | ||
},{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is an oversight from my side, corrected it now, good catch!
|
||
file_pattern = os.path.join(project_path, "ProjectParameters_stage*.json") | ||
stage_files = glob.glob(file_pattern) | ||
print(stage_files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -53,8 +53,10 @@ class GeoMechanicsSchemeTester | |||
mModel.CreateModelPart("dummy", 2); | |||
} | |||
|
|||
template <class T> | |||
void TestFunctionCalledOnComponent_IsCalledOnActiveAndInactiveComponents() | |||
template <class T, typename AddComponentToModelPartLambda> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Richard, it looks very good for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thanks for processing the review comments.
📝 Description
In our workflows which include deactivations + structural elements, we have encountered issues with initialization of inactive elements. This will be fixed in core as well, but until that is done, this temporary fix in the geomechanics time integration scheme is done. Also tests are added (integration + unit), such that we can verify this behavior, also when the fix is done in core and the change in the geomechanics time integration scheme can be reverted.