Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rfaasse
Copy link
Contributor

@rfaasse rfaasse commented Jan 7, 2025

📝 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

@rfaasse rfaasse added the GeoMechanics Issues related to the GeoMechanicsApplication label Jan 7, 2025
@rfaasse rfaasse requested review from avdg81 and WPK4FEM January 7, 2025 12:48
@rfaasse rfaasse self-assigned this Jan 7, 2025
Copy link
Contributor

@avdg81 avdg81 left a 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
Copy link
Contributor

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):

Suggested change
// 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");
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

@WPK4FEM WPK4FEM left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GeoMechanics Issues related to the GeoMechanicsApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GeoMechanicsApplication] Create exit criterion for c-phi process for too small reduction steps
3 participants