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.

Sign up for GitHub

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

Jump to bottom

[GeoMechanicsApplication] Geo/11626 initialize solving strategies settlement #11665

Merged
merged 24 commits into from
Oct 11, 2023
Merged

[GeoMechanicsApplication] Geo/11626 initialize solving strategies settlement #11665

merged 24 commits into from
Oct 11, 2023

Conversation

rfaasse
Copy link
Contributor

@rfaasse rfaasse commented Oct 5, 2023 •
edited
Loading

📝 Description
This PR aims to create the factory for the solving strategy and the necessary subcomponents (Scheme, BuilderAndSolver, ConvergenceCriteria and LinearSolver) and test them using unit tests.

🆕 Changelog

  • Added factories for the SolvingStrategy, Scheme, BuilderAndSolver, ConvergenceCriteria and LinearSolver
  • Added unit tests for all the factories

@rfaasse
Use linear solver factory in solving strategy factory
ca7c635
@rfaasse
Added first version of the complete newton rhapson strategy (hard coded)
540d359
@rfaasse
Added some (placeholder) factories
eafd1a1
@rfaasse
Split the tests into 'fast' unit tests and slower integration tests a…
15687bd 
…nd made sure the run_cpp_unit_tests.py runs both by default and has the option to only run the slowest
@rfaasse
Merge branch 'geo/improve-cpp-test-script-geomechanics' into geo/1162…
76c7c99 
…6_InitializeSolvingStrategies_Settlement
@rfaasse
Added factory for convergence criteria
e334efb
@rfaasse
Added the scheme factory + test and made changes to convergence_crite…
746a953 
…ria_factory
@rfaasse
Remove white line
8575262
@rfaasse
Merge branch 'master' into geo/11626_InitializeSolvingStrategies_Sett…
ff24947 
…lement
@rfaasse
Added builder and solver factory
5d8861f
@rfaasse
Finalized the BuilderAndSolverFactory and added it to the solving_str…
3864eff 
…ategy_factory.cpp
@rfaasse
Merge branch 'master' into geo/11626_InitializeSolvingStrategies_Sett…
46177f1 
…lement
@rfaasse
Converted solving strategy factory to template class
a603a3b
@rfaasse
Imported LinearSolverApplication to register all solvers
27abf1a
@rfaasse
Fix build
c013494
@rfaasse
Minor clean-up
6f717cd
@rfaasse
SQ fix
8112878
@rfaasse
Minor Cleanup
eed8591
@rfaasse rfaasse linked an issue Oct 5, 2023 that may be closed by this pull request
[GeoMechanicsApplication] Initialize the solving strategy for the Settlement Workflow based on project parameters #11626
Closed
@rfaasse rfaasse changed the title Geo/11626 initialize solving strategies settlement [GeoMechanicsApplication] Geo/11626 initialize solving strategies settlement Oct 5, 2023
@rfaasse rfaasse requested review from avdg81, WPK4FEM and mcgicjn2 October 6, 2023 07:01
@rfaasse rfaasse marked this pull request as ready for review October 6, 2023 07:03
avdg81
avdg81 reviewed Oct 9, 2023
View reviewed changes
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 good to me. Nice to have factories that we can extend as we see fit. I just had several minor comments, nothing major.

applications/GeoMechanicsApplication/custom_utilities/builder_and_solver_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/scheme_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_scheme_factory.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_convergence_criteria_factory.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_solving_strategy_factory.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_solving_strategy_factory.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/run_cpp_unit_tests.py Outdated Show resolved Hide resolved
@carloslubbers carloslubbers added the GeoMechanics Issues related to the GeoMechanicsApplication label Oct 10, 2023
rfaasse
rfaasse commented Oct 10, 2023
View reviewed changes
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/run_cpp_unit_tests.py Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/builder_and_solver_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/scheme_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/scheme_factory.hpp Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_solving_strategy_factory.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/scheme_factory.hpp Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_builder_and_solver_factory.cpp Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_builder_and_solver_factory.cpp Outdated Show resolved Hide resolved
@rfaasse rfaasse requested a review from avdg81 October 10, 2023 11:08
avdg81
avdg81 reviewed Oct 10, 2023
View reviewed changes
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.

Thanks for the great work Richard! I still found several minor things that you may want to address.

applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_utilities/solving_strategy_factory.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_builder_and_solver_factory.cpp Outdated
Comment on lines 58 to 60
const auto factory = BuilderAndSolverFactory<SparseSpaceType, LocalSpaceType, LinearSolverType>();
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{}, solver),
"the block_builder parameter is not defined, aborting BuilderAndSolverCreation")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Create is a static member function, you'd better write it like this (just as for the two above test cases):

Suggested change
const auto factory = BuilderAndSolverFactory<SparseSpaceType, LocalSpaceType, LinearSolverType>();
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{}, solver),
"the block_builder parameter is not defined, aborting BuilderAndSolverCreation")
KRATOS_EXPECT_EXCEPTION_IS_THROWN(BuilderAndSolverFactory<SparseSpaceType, LocalSpaceType, LinearSolverType>::Create(Parameters{}, solver),
"the block_builder parameter is not defined, aborting BuilderAndSolverCreation")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree and I tried this before, but for some reason the macro doesn't support it
image

Copy link
Contributor

@avdg81 avdg81 Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you're right. However, you may overcome the above problem by wrapping the first argument of the macro in a pair of parenthesis, i.e.:

Suggested change
const auto factory = BuilderAndSolverFactory<SparseSpaceType, LocalSpaceType, LinearSolverType>();
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{}, solver),
"the block_builder parameter is not defined, aborting BuilderAndSolverCreation")
KRATOS_EXPECT_EXCEPTION_IS_THROWN((BuilderAndSolverFactory<SparseSpaceType, LocalSpaceType, LinearSolverType>::Create(Parameters{}, solver)),
"the block_builder parameter is not defined, aborting BuilderAndSolverCreation")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works indeed, and what also does is creating an alias (which is better for readability anyway), since in this case the problem of the macro seems to be related to having the template arguments there.

applications/GeoMechanicsApplication/tests/cpp_tests/test_scheme_factory.cpp Outdated
Comment on lines 36 to 37
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{parameters}),
"scheme_type is not defined, aborting")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{parameters}),
"scheme_type is not defined, aborting")
KRATOS_EXPECT_EXCEPTION_IS_THROWN(SchemeFactory<SparseSpaceType, LocalSpaceType>::Create(Parameters{parameters}),
"scheme_type is not defined, aborting")

(And then variable factory is no longer needed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as before, the macro doesn't seem to accept it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

applications/GeoMechanicsApplication/tests/cpp_tests/test_scheme_factory.cpp Outdated
Comment on lines 51 to 52
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{parameters}),
"solution_type is not defined, aborting")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as before, the macro doesn't seem to accept it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

applications/GeoMechanicsApplication/tests/cpp_tests/test_scheme_factory.cpp Outdated
Comment on lines 67 to 68
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{parameters}),
"Specified solution_type/scheme_type is not supported, aborting")
Copy link
Contributor

Choose a reason for hiding this comment

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

... and here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as before, the macro doesn't seem to accept it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

applications/GeoMechanicsApplication/tests/cpp_tests/test_solving_strategy_factory.cpp Outdated
KRATOS_TEST_CASE_IN_SUITE(Create_ReturnsSolvingStrategy_ForNewtonRhapsonStrategy, KratosGeoMechanicsFastSuite)
{
Model model;
const int buffer_size = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

As Wijtze Pieter suggested, let's change this value to 2 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

applications/GeoMechanicsApplication/tests/cpp_tests/test_solving_strategy_factory.cpp Outdated
parameters, dummy_model_part);

KRATOS_EXPECT_NE(created_strategy, nullptr);
created_strategy->Check();
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this function call added intentionally? If yes, then I would have expected some kind of checking. Perhaps its return value. Or whether it throws (or not). The way it is written now confuses me a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I implicitly check if it throws by just having it there. I added the check for the return value.

@rfaasse rfaasse requested a review from avdg81 October 11, 2023 07:06
rfaasse
rfaasse commented Oct 11, 2023
View reviewed changes
Copy link
Contributor Author

@rfaasse rfaasse left a comment

Choose a reason for hiding this comment

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

Got the static calls in, feel free to re-review

applications/GeoMechanicsApplication/tests/cpp_tests/test_builder_and_solver_factory.cpp Outdated
Comment on lines 58 to 60
const auto factory = BuilderAndSolverFactory<SparseSpaceType, LocalSpaceType, LinearSolverType>();
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{}, solver),
"the block_builder parameter is not defined, aborting BuilderAndSolverCreation")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works indeed, and what also does is creating an alias (which is better for readability anyway), since in this case the problem of the macro seems to be related to having the template arguments there.

applications/GeoMechanicsApplication/tests/cpp_tests/test_convergence_criteria_factory.cpp Outdated
Comment on lines 43 to 45
const ConvergenceCriteriaFactory<SparseSpaceType, LocalSpaceType> factory;

KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{"{}"}), "No convergence_criterion is defined, aborting.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

applications/GeoMechanicsApplication/tests/cpp_tests/test_convergence_criteria_factory.cpp Outdated
Comment on lines 50 to 52
const ConvergenceCriteriaFactory<SparseSpaceType, LocalSpaceType> factory;

KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{R"({"convergence_criterion" : "something_unknown" })"}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

applications/GeoMechanicsApplication/tests/cpp_tests/test_scheme_factory.cpp Outdated
Comment on lines 36 to 37
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{parameters}),
"scheme_type is not defined, aborting")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

applications/GeoMechanicsApplication/tests/cpp_tests/test_scheme_factory.cpp Outdated
Comment on lines 51 to 52
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{parameters}),
"solution_type is not defined, aborting")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

applications/GeoMechanicsApplication/tests/cpp_tests/test_scheme_factory.cpp Outdated
Comment on lines 67 to 68
KRATOS_EXPECT_EXCEPTION_IS_THROWN(factory.Create(Parameters{parameters}),
"Specified solution_type/scheme_type is not supported, aborting")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using your suggestion

avdg81
avdg81 approved these changes Oct 11, 2023
View reviewed changes
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 good and clear to me. Please merge.

@rfaasse rfaasse merged commit 9d63d6a into master Oct 11, 2023
@rfaasse rfaasse deleted the geo/11626_InitializeSolvingStrategies_Settlement branch October 11, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avdg81 avdg81 avdg81 approved these changes

@WPK4FEM WPK4FEM Awaiting requested review from WPK4FEM

@mcgicjn2 mcgicjn2 Awaiting requested review from mcgicjn2

Assignees
No one assigned
Labels
GeoMechanics Issues related to the GeoMechanicsApplication
Projects
None yet
Milestone
No milestone
3 participants
@rfaasse @avdg81 @carloslubbers