-
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] Geo/linear elastic strategy #12479
base: master
Are you sure you want to change the base?
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.
dont forget to revert this
# Conflicts: # applications/GeoMechanicsApplication/custom_conditions/U_Pw_normal_lysmer_absorbing_condition.hpp # applications/GeoMechanicsApplication/custom_python/add_custom_processes_to_python.cpp # applications/GeoMechanicsApplication/custom_python/add_custom_strategies_to_python.cpp # applications/GeoMechanicsApplication/python_scripts/geomechanics_analysis.py
# Conflicts: # applications/GeoMechanicsApplication/custom_conditions/U_Pw_normal_lysmer_absorbing_condition.cpp
void AddDynamicsToLhs(TSystemMatrixType& rA, const ModelPart& rModelPart) | ||
{ | ||
const double delta_time = rModelPart.GetProcessInfo()[DELTA_TIME]; | ||
|
||
double* a_values = rA.value_data().begin(); | ||
const double* m_values = mMassMatrix.value_data().begin(); | ||
const double* c_values = mDampingMatrix.value_data().begin(); | ||
|
||
// add mass and damping contribution to LHS sparse matrix | ||
// mass contribution: 1.0 / (mBeta * delta_time * delta_time) * M | ||
// damping contribution: mGamma / (mBeta * delta_time) * C | ||
for (std::size_t i = 0; i < rA.size1(); i++) { | ||
const std::size_t col_begin = rA.index1_data()[i]; | ||
const std::size_t col_end = rA.index1_data()[i + 1]; | ||
|
||
for (std::size_t j = col_begin; j < col_end; ++j) { | ||
a_values[j] += (1.0 / (mBeta * delta_time * delta_time)) * m_values[j]; | ||
a_values[j] += (mGamma / (mBeta * delta_time)) * c_values[j]; | ||
} | ||
} | ||
} |
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 there is quite a bit of overlap with the time integration scheme and this builder and solver takes a lot of responsibility which the schemes normally have. E.g. compare this function with the function with the same name for the scheme (see snippet below). I think it's worth exploring if we can re-use functionality here, since a lot of the BuilderAndSolver functions have access to the scheme.
Lines 266 to 283 in 1166a00
void AddDynamicsToLHS(LocalSystemMatrixType& LHS_Contribution, | |
LocalSystemMatrixType& M, | |
LocalSystemMatrixType& C, | |
const ProcessInfo& CurrentProcessInfo) | |
{ | |
KRATOS_TRY | |
// adding mass contribution | |
if (M.size1() != 0) | |
noalias(LHS_Contribution) += | |
(1.0 / (this->GetBeta() * this->GetDeltaTime() * this->GetDeltaTime())) * M; | |
// adding damping contribution | |
if (C.size1() != 0) | |
noalias(LHS_Contribution) += (this->GetGamma() / (this->GetBeta() * this->GetDeltaTime())) * C; | |
KRATOS_CATCH("") | |
} |
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.
Even if that's not possible, I'd prefer we don't loop over all elements, but just re-use the ublas functionality
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 function as i implented loops over the sparse matrix, therefore, there are less operations compared to the function in the scheme.
what do you mean by "re-use the ublas functionality"?
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.
furthemore AddDynamicsToLHS is a protected method which i cannot acces from here
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 function as i implented loops over the sparse matrix, therefore, there are less operations compared to the function in the scheme.
what do you mean by "re-use the ublas functionality"?
I would expect ublas to have efficient functionality for sparse matrices, such that we could put the following instead of the loops:
rA += 1.0/(mBeta * delta_time * delta_time) * mMassMatrix
rA += (mGamma / (mBeta * delta_time)) * mDampingMatrix
However, I'm not sure if that's implemented in the efficient way you'd like (and if there are other e.g. size-related issues here), but I think for readability's sake it's worth it to try and see if it doesn't impact performance.
furthemore AddDynamicsToLHS is a protected method which i cannot acces from here
Indeed, however we might be able to override one of the scheme functions in a new scheme, where we expose this functionality. Since it's generally the scheme that's doing this, I would prefer to keep it there (although we indeed might need adjustments).
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.
from what i read on the internet is that the ublas functionality for matrix multiplication is not super efficient. My code is similar as in the base class, which I assume is done for a reason
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 my opinion the code is much more readable (we don't need the comments, because the code itself just shows the formula), so could we just try it? If the performance drops significantly, we can revert it, but it's worth checking in case it is similar.
...rategies/builder_and_solvers/residualbased_block_builder_and_solver_linear_elastic_dynamic.h
Show resolved
Hide resolved
...rategies/builder_and_solvers/residualbased_block_builder_and_solver_linear_elastic_dynamic.h
Outdated
Show resolved
Hide resolved
...rategies/builder_and_solvers/residualbased_block_builder_and_solver_linear_elastic_dynamic.h
Outdated
Show resolved
Hide resolved
void CalculateReactions(typename TSchemeType::Pointer pScheme, | ||
ModelPart& rModelPart, | ||
TSystemMatrixType& A, | ||
TSystemVectorType& Dx, | ||
TSystemVectorType& b) override | ||
{ | ||
TSparseSpace::SetToZero(b); | ||
|
||
// refresh RHS to have the correct reactions | ||
this->BuildRHSNoDirichlet(pScheme, rModelPart, b); | ||
|
||
// NOTE: dofs are assumed to be numbered consecutively in the BlockBuilderAndSolver | ||
block_for_each(BaseType::mDofSet, [&](Dof<double>& rDof) { | ||
const std::size_t i = rDof.EquationId(); | ||
|
||
rDof.GetSolutionStepReactionValue() = -b[i]; | ||
}); | ||
} |
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 don't see a difference between this and the version of the base, so I think we can just remove this override
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 difference is that the underlying private functions are not equal as in the baseclass
...ions/GeoMechanicsApplication/tests/cpp_tests/test_newton_raphson_strategy_linear_elastic.cpp
Outdated
Show resolved
Hide resolved
...ions/GeoMechanicsApplication/tests/cpp_tests/test_newton_raphson_strategy_linear_elastic.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_utilities/geo_custom_condition.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_utilities/geo_custom_condition.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_utilities/geo_custom_condition.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_utilities/geo_custom_condition.h
Outdated
Show resolved
Hide resolved
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 is a nice performance improvement for cases where the left hand side is constant and thanks for adding the unit/integration tests, which help to understand the purpose of this feature.
Next to the comments more focused on detail (part of them can already be tackled), I think there are a few topics that should be addressed before we can merge this PR:
- The new block builder and solver, as well as the solving strategy seem to have taken on too much responsibility. The Newmark beta/gamma updates are duplicated now, I think it's important to explore ways to re-use what's already there.
- Ideally the new builder and solver and solving strategy only differ from their bases in the changed functionality. There is now still quite some overlap which I think can be avoided. That will also make it more easy to review these parts and understand what the exact differences are.
Unfortunately, I don't have quick fixes for most of these duplications. I would propose to first fix the straight-forward comments and think about how you could re-use the scheme functionality and the base builder/solver functionality. We will also have a look from our side, to see what concrete steps we can take to make a start with addressing these issues. We can discuss your and our ideas in a meeting later.
Lastly, I looked at this PR from a code perspective and although the overall goal is clear, I think it's important that @WPK4FEM also takes a look from the FEM perspective.
applications/GeoMechanicsApplication/tests/cpp_tests/test_utilities/geo_custom_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_utilities/geo_custom_element.h
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_utilities/geo_custom_condition.h
Outdated
Show resolved
Hide resolved
"rayleigh_m": 0.02, | ||
"rayleigh_k": 6e-6, | ||
"strategy_type": "newton_raphson_linear_elastic", | ||
"convergence_criterion": "displacement_criterion", |
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.
Do we need to check that this method also works for 'residual_criterion'?
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.
im not sure if that is relevant as i've changed nothing for the convergence criteria
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, but there are still functions copied from the base and moved around, so I think it's a good idea to use residual criterion for one of the test cases (shouldn't change the results anyway, just a json change), just to make sure. But I leave it up to you 👍
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.
Could you add a README.md for these tests? We try to add documentation for new tests (and are also going through existing ones to add it). That makes it a lot easier to know what the test is about (like in the base case: https://github.com/KratosMultiphysics/Kratos/blob/99356f467faab1464f2046d9c6535442d57df874/applications/GeoMechanicsApplication/tests/test_1d_wave_prop_drained_soil/README.md)
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 would still be a good addition
new_builder_and_solver = GeoMechanicsApplication.ResidualBasedBlockBuilderAndSolverLinearElasticDynamic( | ||
self.linear_solver, | ||
beta, | ||
gamma, | ||
calculate_initial_acceleration) | ||
|
||
solving_strategy = GeoMechanicsApplication.GeoMechanicNewtonRaphsonStrategyLinearElasticDynamic( | ||
self.computing_model_part, | ||
self.scheme, | ||
self.linear_solver, | ||
self.convergence_criterion, | ||
new_builder_and_solver, | ||
self.strategy_params, | ||
beta, | ||
gamma, | ||
max_iters, | ||
compute_reactions, | ||
move_mesh_flag) |
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.
Here it seems strange that the block builder and solver, as well as the solving strategy both need to know about beta and gamma (which generally only the time integration scheme cares about). I think we should explore if we can re-use the functionality of the newmark (dynamic) upw scheme, without duplicating the time integration functionality in these two additional places.
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.
as mentioned in above comments, I dont think its always possible to reuse the newmark (dynamic) upw scheme, but perhaps a new scheme can be added. But the builder_and_solver certainly needs the beta and gamma, as the behaviour in the builder_and_solver cannot be captured in the scheme
...ions/GeoMechanicsApplication/tests/cpp_tests/test_newton_raphson_strategy_linear_elastic.cpp
Outdated
Show resolved
Hide resolved
...ustom_strategies/strategies/residualbased_newton_raphson_strategy_linear_elastic_dynamic.hpp
Show resolved
Hide resolved
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 @aronnoordam, thanks for processing the first round of feedback. This helps a lot in understanding the code and giving better feedback and in my opinion both the tests and production code improved a lot: less duplication, clearer responsibilities of classes and more specific testing on what the strategy does and (more importantly) does not do/call.
This round, I tried to be more detailed in my comments, due to larger scale design becoming more clear. For the builder and solver, I would like to discuss a bit more with the team about if it makes sense to move more newmark functionality to the scheme and reduce a bit more duplication (which is hard as you mentioned, due to the differences in private non-virtual functions).
Next to that, @avdg81 and @WPK4FEM will also have another look at the PR 👍
: ResidualBasedNewtonRaphsonStrategy<TSparseSpace, TDenseSpace, TLinearSolver>( | ||
rModelPart, pScheme, pNewConvergenceCriteria, pNewBuilderAndSolver, MaxIterations, CalculateReactions, false, MoveMeshFlag) | ||
{ | ||
// new constructor |
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 comment can be removed
|
||
BaseType::GetScheme()->Predict(BaseType::GetModelPart(), r_dof_set, rA, rDx, rb); | ||
|
||
// Note that constraints are not applied in this predict, nor is an update performed |
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.
Maybe it's good to mention here that the constraints are applied in the builder and solver.
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.
And did you already find what the reason is that the constraints cannot be applied in the same way as for the base strategy?
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.
yes actually, as for constraints, there is a dotproduct with the constraint transformation matrix and the RHS, now in my case, i already transformed the mass and damping matrix, thus when applying a constraint again to the rhs, part of the RHS is transformed twice.
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.
Since the formulations are different for this new incremental scheme, this should be documented in the README.md with the other schemes.
} | ||
|
||
if (is_converged) { | ||
// here only the derivatives are updated |
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 it's good to add here that only the derivatives are updated in the specific scheme we're using. In general schemes will update both the derivatives and the instance variable
void AddDynamicsToLhs(TSystemMatrixType& rA, const ModelPart& rModelPart) | ||
{ | ||
const double delta_time = rModelPart.GetProcessInfo()[DELTA_TIME]; | ||
|
||
double* a_values = rA.value_data().begin(); | ||
const double* m_values = mMassMatrix.value_data().begin(); | ||
const double* c_values = mDampingMatrix.value_data().begin(); | ||
|
||
// add mass and damping contribution to LHS sparse matrix | ||
// mass contribution: 1.0 / (mBeta * delta_time * delta_time) * M | ||
// damping contribution: mGamma / (mBeta * delta_time) * C | ||
for (std::size_t i = 0; i < rA.size1(); i++) { | ||
const std::size_t col_begin = rA.index1_data()[i]; | ||
const std::size_t col_end = rA.index1_data()[i + 1]; | ||
|
||
for (std::size_t j = col_begin; j < col_end; ++j) { | ||
a_values[j] += (1.0 / (mBeta * delta_time * delta_time)) * m_values[j]; | ||
a_values[j] += (mGamma / (mBeta * delta_time)) * c_values[j]; | ||
} | ||
} | ||
} |
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 my opinion the code is much more readable (we don't need the comments, because the code itself just shows the formula), so could we just try it? If the performance drops significantly, we can revert it, but it's worth checking in case it is similar.
"rayleigh_m": 0.02, | ||
"rayleigh_k": 6e-6, | ||
"strategy_type": "newton_raphson_linear_elastic", | ||
"convergence_criterion": "displacement_criterion", |
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, but there are still functions copied from the base and moved around, so I think it's a good idea to use residual criterion for one of the test cases (shouldn't change the results anyway, just a json change), just to make sure. But I leave it up to you 👍
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.
Could you also rename the file-name (.cpp and .h) to geo_mock_element to align with the class name?
// | ||
// Main authors: Aron Noordam | ||
// | ||
#include "custom_utilities/dof_utilities.h" |
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.
Could we just include dof.h instead of dof_utilities? or are we using something from the utils?
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.
will check
std::vector<double> expected_displacement_x = {0.00673, 0.0505, 0.189, 0.485, 0.961, 1.58, | ||
2.23, 2.76, 3.0, 2.85, 2.28, 1.40}; | ||
std::vector<double> expected_displacement_y = {0.364, 1.35, 2.68, 4.00, 4.95, 5.34, | ||
5.13, 4.48, 3.64, 2.90, 2.44, 2.31}; | ||
|
||
double delta_time = 0.28; | ||
bool use_iterations = false; | ||
bool calculate_initial_acceleration = true; | ||
bool use_direct_solver = 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.
These can all be const (same holds for the other tests)
KRATOS_EXPECT_EQ(geo_custom_element.GetCountCalculateLeftHandSideCalled(), 1); | ||
KRATOS_EXPECT_EQ(geo_custom_element.GetCountCalculateMassMatrixCalled(), 1); | ||
KRATOS_EXPECT_EQ(geo_custom_element.GetCalculateDampingMatrixCalled(), 1); | ||
KRATOS_EXPECT_EQ(geo_custom_element.GetCountCalculateRightHandSideCalled(), 0); | ||
|
||
KRATOS_EXPECT_EQ(geo_custom_condition.GetCountCalculateLeftHandSideCalled(), 1); | ||
KRATOS_EXPECT_EQ(geo_custom_condition.GetCountCalculateMassMatrixCalled(), 1); | ||
KRATOS_EXPECT_EQ(geo_custom_condition.GetCalculateDampingMatrixCalled(), 1); | ||
|
||
// rhs for conditions is called each solution step and during initialisation | ||
if (UseIterations) { | ||
// Two iterations are performed per solution step, thus rhs for conditions is called twice each solution step step and during initialisation | ||
KRATOS_EXPECT_EQ(geo_custom_condition.GetCountCalculateRightHandSideCalled(), n_steps * 2 + 1); | ||
} else { | ||
KRATOS_EXPECT_EQ(geo_custom_condition.GetCountCalculateRightHandSideCalled(), n_steps + 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.
Thanks for adding these checks, it also functions as documentation (it's really clear what is called how many times from this 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.
This is a big piece of work that, as I understood from you, yields a significant performance improvement, which is great. As you can see, I have several suggestions. For me the most important ones are aimed at making the new scheme's initialization and finalization behavior symmetric. Also, I have a suggestion to remove the duplication of the nonlinear iteration, which nicely ties in with my previous suggestion. I hope my suggestions will contribute to making your code clearer and more maintainable. Feel free to reach out when my comments are unclear or when you believe that some of them are not feasible. Thanks.
typename TLinearSolver::Pointer pNewLinearSolver, | ||
typename TConvergenceCriteriaType::Pointer pNewConvergenceCriteria, | ||
typename TBuilderAndSolverType::Pointer pNewBuilderAndSolver, | ||
Parameters& rParameters, |
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 addition, also pNewLinearSolver
is no longer (?) used. So I would suggest to remove it as well (including the corresponding Doxygen description).
*/ | ||
void Initialize() override | ||
{ | ||
KRATOS_TRY; |
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.
Please remove the empty statement:
KRATOS_TRY; | |
KRATOS_TRY |
// initialize the system matrices and the initial second derivative | ||
this->InititalizeSystemAndState(); | ||
|
||
KRATOS_CATCH(""); |
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.
Please remove the empty statement:
KRATOS_CATCH(""); | |
KRATOS_CATCH("") |
{ | ||
KRATOS_TRY; | ||
|
||
BaseType::Initialize(); |
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 implementation of the base class's Initialize
suggests that it may be called more than once, since there is a flag (mInitializeWasPerformed
) that makes sure the actual initialization is performed only once. Would we need a similar guarding mechanism here?
|
||
BaseType::Initialize(); | ||
|
||
// Note that FindNeighbourElementsOfConditionsProcess and DeactivateConditionsOnInactiveElements are required to be perfomed before initializing the System and State |
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.
A typo:
// Note that FindNeighbourElementsOfConditionsProcess and DeactivateConditionsOnInactiveElements are required to be perfomed before initializing the System and State | |
// Note that FindNeighbourElementsOfConditionsProcess and DeactivateConditionsOnInactiveElements are required to be performed before initializing the System and State |
|
||
DofsArrayType& r_dof_set = BaseType::GetBuilderAndSolver()->GetDofSet(); | ||
|
||
BaseType::GetScheme()->Predict(BaseType::GetModelPart(), r_dof_set, rA, rDx, rb); |
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.
Since rA
, rDx
, and rb
are only used once, I'd suggest to eliminate this local variables:
BaseType::GetScheme()->Predict(BaseType::GetModelPart(), r_dof_set, rA, rDx, rb); | |
BaseType::GetScheme()->Predict(BaseType::GetModelPart(), r_dof_set, *BaseType::mpA, *BaseType::mpDx, *BaseType::mpb); |
You could take this one step further and eliminate r_dof_set
in the same way.
KRATOS_CATCH("") | ||
} | ||
|
||
void FinalizeNonLinIteration(ModelPart& rModelPart, TSystemMatrixType&, TSystemVectorType&, TSystemVectorType&) override |
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 would suggest to make the initialization and finalization of a nonlinear iteration symmetric. At present, it isn't, because the member function that initializes a nonlinear iteration initializes both the elements and the conditions. (See GeoMechanicsTimeIntegrationScheme<...>::InitializeNonLinIteration
). My suggestion would be to add an override
for InitializeNonLinIteration
that only initializes the conditions. Then in your new strategy, you can initialize the elements prior to carrying out the first nonlinear iteration.
block_for_each(r_model_part.Conditions(), [&r_current_process_info](Condition& r_condition) { | ||
if (r_condition.IsActive()) { | ||
r_condition.FinalizeNonLinearIteration(r_current_process_info); | ||
} | ||
}); |
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.
If I'm not mistaken, this is exactly what your new scheme does...? So I think you can replace it by:
block_for_each(r_model_part.Conditions(), [&r_current_process_info](Condition& r_condition) { | |
if (r_condition.IsActive()) { | |
r_condition.FinalizeNonLinearIteration(r_current_process_info); | |
} | |
}); | |
p_scheme->FinalizeNonLinIteration(r_model_part, rA, rDx, rb); |
bool PerformIterationCycle(TSystemMatrixType& rA, | ||
TSystemVectorType& rDx, | ||
TSystemVectorType& rb, | ||
TSystemVectorType& rDxTot, | ||
std::vector<Vector>& rNonconvergedSolutions, | ||
unsigned int& rIterationNumber) |
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 would suggest to extract a member function that does a single nonlinear iteration, and then reuse that for the first iteration as well as for any subsequent iterations. In terms of your implementation, it would roughly look like this:
r_model_part.GetProcessInfo()[NL_ITERATION_NUMBER] = rIterationNumber;
p_scheme->InitializeNonLinIteration(r_model_part, rA, rDx, rb);
BaseType::mpConvergenceCriteria->InitializeNonLinearIteration(r_model_part, r_dof_set, rA, rDx, rb);
is_converged = BaseType::mpConvergenceCriteria->PreCriteria(r_model_part, r_dof_set, rA, rDx, rb);
// call the linear system solver to find the correction mDx for the
// it is not called if there is no system to solve
if (SparseSpaceType::Size(rDx) != 0) {
TSparseSpace::SetToZero(rDx);
TSparseSpace::SetToZero(rb);
p_builder_and_solver->BuildRHSAndSolve(p_scheme, r_model_part, rA, rDx, rb);
} else {
KRATOS_WARNING("NO DOFS") << "ATTENTION: no free DOFs!! " << std::endl;
}
// Debugging info
BaseType::EchoInfo(rIterationNumber);
// Updating the results stored in the database
this->UpdateSolutionStepValue(rDx, rDxTot);
p_scheme->FinalizeNonLinIteration(r_model_part, rA, rDx, rb);
BaseType::mpConvergenceCriteria->FinalizeNonLinearIteration(r_model_part, r_dof_set, rA, rDx, rb);
if (BaseType::mStoreNonconvergedSolutionsFlag) {
Vector ith;
BaseType::GetCurrentSolution(r_dof_set, ith);
rNonconvergedSolutions.push_back(ith);
}
if (is_converged) {
if (BaseType::mpConvergenceCriteria->GetActualizeRHSflag()) {
TSparseSpace::SetToZero(rb);
p_builder_and_solver->BuildRHS(p_scheme, r_model_part, rb);
}
is_converged =
BaseType::mpConvergenceCriteria->PostCriteria(r_model_part, r_dof_set, rA, rDx, rb);
}
This will take away quite a bit of duplication.
// only initialize conditions, not that this cannot be put in the scheme, as the scheme is required to InitializeNonLinearIteration both elements and conditions | ||
const auto& r_current_process_info = r_model_part.GetProcessInfo(); | ||
block_for_each(r_model_part.Conditions(), [&r_current_process_info](Condition& r_condition) { | ||
if (r_condition.IsActive()) { | ||
r_condition.InitializeNonLinearIteration(r_current_process_info); | ||
} | ||
}); |
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 would suggest to do the initialization of the elements here, and leave the initialization of the conditions to new your new scheme (see one of my other suggestions). That would make you new scheme symmetric in terms of initialization and finalization. But it also highlights that the handling of elements is special and needs to be done only once rather than in each nonlinear iteration.
…vent calculation of internal forces
…uasi static scheme
added another strategy and builder and solver meant for linear elastic problems.
Mostly, this strategy exploits the fact that the internal forces dont have to be recalculated every iteration.
Only external forces are updated