-
Notifications
You must be signed in to change notification settings - Fork 247
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] Create exit criterion for c phi process #12980
[GeoMechanicsApplication] Create exit criterion for c phi process #12980
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.
This looks like a clean and straightforward improvement with nice tests. I have only one very minor suggestion and a question. None of them is blocking.
} | ||
|
||
// After halving the initial reduction increment (0.1) for the seventh time, it becomes | ||
// 0,00078125, so it should throw an exception |
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.
Nitpicking (use the dot as decimal separator):
// 0,00078125, so it should throw an exception | |
// 0.00078125, so it should throw an exception |
// 0,00078125, so it should throw an exception | ||
KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
process.ExecuteInitializeSolutionStep(), | ||
"Reduction increment should not drop below 0.001, calculation stopped. Final safety factor = 1"); |
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 was a bit surprised to see a final safety factor of 1.0. I'm not sure whether I would have expected that particular value. Can you perhaps help me to see why 1.0 must be the correct value in this case?
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.
Indeed. the error message use mPreviousReductionFactor which is only updated in ExecuteFinalizeSolutionStep.
process.ExecuteFinalizeSolutionStep is not invoked in the test, hence the factor of safety remained 1.
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, I left it out to keep the test simpler, but now it isn't really testing the reporting of the final safety factor. Changed the test
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.
Dear Richard,
Thank you for making this more robust. I have no other remarks than those that Anne already raised, but indicated how the safetyfactor remaining at 1. can be cured for the unit test.
Regards, Wijtze Pieter
…er review comments
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.
Looks good to me. Thank you for including the finalize.
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.
Thanks for the reviews, processed the comments!
// 0,00078125, so it should throw an exception | ||
KRATOS_EXPECT_EXCEPTION_IS_THROWN( | ||
process.ExecuteInitializeSolutionStep(), | ||
"Reduction increment should not drop below 0.001, calculation stopped. Final safety factor = 1"); |
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, I left it out to keep the test simpler, but now it isn't really testing the reporting of the final safety factor. Changed the test
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.
Thank you very much for taking into account our suggestions. I have no further comments or suggestions, so please feel free to merge this PR.
📝 Description
There was no exit criterium for c-phi for a small reduction increment (i.e. very small changes to c and phi). This PR adds it