-
Notifications
You must be signed in to change notification settings - Fork 249
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] Convert two stage line loads test to the orchestrator based analysis and test #13057
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.
Thank you so much for investigating how to convert the original two-stage test to a test that uses the orchestrator. The result of your efforts show clearly what is needed to run multi-stage analysis with this novel approach. Well done!
I have a few suggestions that you may find helpful.
...hanicsApplication/tests/line_load_tests/line_loads_with_orchestrator/MaterialParameters.json
Outdated
Show resolved
Hide resolved
...hanicsApplication/tests/line_load_tests/line_loads_with_orchestrator/MaterialParameters.json
Outdated
Show resolved
Hide resolved
...cations/GeoMechanicsApplication/tests/line_load_tests/line_loads_with_orchestrator/README.md
Outdated
Show resolved
Hide resolved
...cations/GeoMechanicsApplication/tests/line_load_tests/line_loads_with_orchestrator/README.md
Outdated
Show resolved
Hide resolved
...hanicsApplication/tests/line_load_tests/line_loads_with_orchestrator/MaterialParameters.json
Outdated
Show resolved
Hide resolved
Hi @rubenzorrilla , here is our first case with the orchestrator. Please could you have a brief look at it? Perhaps, you may suggest some improvements. Thank 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.
Thank you for processing the review suggestions. I have one more tiny suggestion, and then I think we're ready to merge your work. Thank you for preparing this clear test using the orchestrator-based approach for multi-stage analysis.
@@ -218,6 +218,9 @@ def RunSolutionLoop(self): | |||
self.FinalizeSolutionStep() | |||
self.OutputSolutionStep() | |||
|
|||
# For orchestrator-based multi-stage analyses, we need to register our own analysis stage class | |||
Kratos.Registry.AddItem("Stages.KratosMultiphysics.GeoMechanicsApplication.geomechanics_analysis.GeoMechanicsAnalysis.ClassName", "GeoMechanicsAnalysis") | |||
Kratos.Registry.AddItem("Stages.KratosMultiphysics.GeoMechanicsApplication.geomechanics_analysis.GeoMechanicsAnalysis.ModuleName", "KratosMultiphysics.GeoMechanicsApplication.geomechanics_analysis") |
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 add one (or perhaps even two) blank line(s) after these lines, to clearly separate them from the next line (if __name__ == '__main__':
), since they are unrelated.
# For orchestrator-based multi-stage analyses, we need to register our own analysis stage class | ||
Kratos.Registry.AddItem("Stages.KratosMultiphysics.GeoMechanicsApplication.geomechanics_analysis.GeoMechanicsAnalysis.ClassName", "GeoMechanicsAnalysis") | ||
Kratos.Registry.AddItem("Stages.KratosMultiphysics.GeoMechanicsApplication.geomechanics_analysis.GeoMechanicsAnalysis.ModuleName", "KratosMultiphysics.GeoMechanicsApplication.geomechanics_analysis") |
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 should be placed in the registry lists to happen when importing the application module. Note that current implementation implies that the happens when importing the stage module, something that precludes using the registry as a factory.
See an example in the FluidDynamicsApplication
here and 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.
Thank you for all the hard work you've done. I believe this PR is ready to be merged.
📝 Description
A brief description of the PR.