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] Support residual_criterion in c++ interface #11833

Merged
merged 6 commits into from
Nov 23, 2023

Conversation

carloslubbers
Copy link
Contributor

@carloslubbers carloslubbers commented Nov 22, 2023

This PR adds support for another convergence criterion, including the following changes:

  • Support residual_criterion in c++ interface
  • Added test for new criterion and extracted some shared functionality
  • Slightly changed function in parameters_utilities.cpp such that it is usable in the factory (and other factories)
  • Removed duplicate functionality from solving_strategy_factory.hpp

WPK4FEM
WPK4FEM previously approved these changes Nov 23, 2023
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.

Meets the objective of making residual_criterion available and having a unit test for it.

Comment on lines 25 to 35

for (const std::string& entry : rNamesOfParametersToCopy)
{
if (rSourceParameters.Has(entry))
{
result.AddValue(entry, rSourceParameters[entry]);
}
}

return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that this is now shared i.s.o. copied.

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.

Looks like a clear and clean extension of existing functionality. I also like it a lot that you have eliminated some duplicated code. Well done! I only have a few minor suggestions, nothing blocking.

const std::vector<std::string> entries_to_copy = {
"displacement_absolute_tolerance",
"displacement_relative_tolerance"};
Parameters convergence_inputs = ParametersUtilities::ExtractParameters(rSolverSettings, entries_to_copy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this variable const:

Suggested change
Parameters convergence_inputs = ParametersUtilities::ExtractParameters(rSolverSettings, entries_to_copy);
const auto convergence_inputs = ParametersUtilities::ExtractParameters(rSolverSettings, entries_to_copy);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, for some other constructors this was not possible, but here it seems okay

Comment on lines +25 to +33

for (const std::string& entry : rNamesOfParametersToCopy)
{
if (rSourceParameters.Has(entry))
{
result.AddValue(entry, rSourceParameters[entry]);
}
}

Copy link
Contributor

@avdg81 avdg81 Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have changed the behavior of this function, I'm pondering whether we should rename it to indicate that it will only copy parameters that exist in rSourceParameters. The supplied parameter names don't have to be present there and they are silently skipped if they don't. The only suggestion that comes to my mind now is CopyOptionalParameters. (Its counterpart could then be named CopyRequiredParameters.) What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I brought back the old function so we can use both

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.

We are very happy with these changes, so please merge this PR.

@rfaasse rfaasse merged commit 890d1a2 into master Nov 23, 2023
16 checks passed
@rfaasse rfaasse deleted the geo/Add-Residual-Criterion branch November 23, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants