[GeoMechanicsApplication] Clean up strategies #12792
Conversation
The override just called the base class's version, which yields the same behavior as not providing an override.
The moved member function was used by one derived class only. The access specifier has been changed from `protected` to `private`.
The moved data members were used by one derived class only. The access specifier has been changed from `protected` to `private`.
Also reformatted the code with clang-format.
Also, "unnamed" a few other function parameters that are no longer used.
avdg81
force-pushed
the
geo/clean-up-newton-raphson-strategy
branch
from
54a4489 to
f0685a5
Compare
These changes address three code smells that were found by SonarQube.
| auto get_dof_value = [](const auto& rDof) { return rDof.GetSolutionStepValue(); }; | ||
| std::transform(std::begin(free_dofs), std::end(free_dofs), free_dof_values.begin(), get_dof_value); | ||
|
|
||
| return norm_2(free_dof_values); |
| } | ||
|
|
||
| std::vector<ModelPart*> mSubModelPartList; // List of every SubModelPart associated to an external load | ||
| std::vector<std::string> mVariableNames; // Name of the nodal variable associated to every SubModelPart |
There was a problem hiding this comment.
Was this used anywhere in the end?
There was a problem hiding this comment.
Yes, I've found that these data members are being used. See member function UpdateExternalLoads, which uses the lists of model part pointers and variable names. Unfortunately, there are no tests that cover this code :-(
| static double CalculateReferenceDofsNorm(DofsArrayType& rDofSet) | ||
| { | ||
| auto is_free_dof = [](const auto& rDof) { return rDof.IsFree(); }; | ||
| auto free_dofs = rDofSet | boost::adaptors::filtered(is_free_dof); | ||
|
|
||
| auto free_dof_values = Vector{boost::size(free_dofs)}; | ||
| auto get_dof_value = [](const auto& rDof) { return rDof.GetSolutionStepValue(); }; | ||
| std::transform(std::begin(free_dofs), std::end(free_dofs), free_dof_values.begin(), get_dof_value); | ||
|
|
||
| return norm_2(free_dof_values); | ||
| } |
There was a problem hiding this comment.
At some point, we could move this to a utility (it doesn't seem to be very arc length specific, but I might be wrong)
There was a problem hiding this comment.
You are correct that this calculation of the L2 norm of the (free) degrees of freedom is not specific for arc-length control. So it could be extracted to a more generic utility function and then be called from here. However, as we discussed, if we'd like to do that, let's do it in a separate PR.
📝 Description
Cleaned up the strategies that are offered by the GeoMechanicsApplication.
🆕 Changelog
Changes include:
GeoMechanicsNewtonRaphsonStrategy) used to receive a pointer to a linear solver, but that parameter was never used. Consequently, there is no point in supplying one. In fact, requesting one but not using it is very confusing. The constructors of the derived classes simply forwarded the linear solver pointers to the base class constructor. Also there, the corresponding parameters have been removed from the constructors.GeoMechanicsNewtonRaphsonStrategyto one of the derived classes (GeoMechanicsRammArcLengthStrategy), since only there they were actually being used. As a result, the moved members could be madeprivaterather thanprotected.norm_2function. There is one thing we need to keep in mind here: the local implementation used to be done in parallel (although it was using deprecated functionality to achieve that). In other words, there might be a performance impact here. The changes made will probably also fix three code smells found by SonarQube.