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

[GeoMechanics] GeoMechanics with Reduced Order Models using Kratos RomApplication #11785

Merged
merged 9 commits into from
Nov 21, 2023

Conversation

Rbravo555
Copy link
Member

This PR implements the required changes in two python scripts of the GeoMechanics application to conform with the format expected by the RomApplication.

📝 Description
The RomApplication works with other applications in Kratos by keeping the behaviour of the specific app, while changing only the builder and solver. To do this, the format inside the scripts should match the expectations by the RomApp. For the GeoMechanics application, changes to two files were required:

  • In geo_mechanics_solver.py the required functions should be named _CreateBuilderAndSolver rather than _ConstructBuilderAndSolver, and there should be a function returning the linear solver.

  • In geomechanics_solvers_wrapper.py, the mechanism inside the RomApp for setting the RomBuilderAndSolver requires the existance of the CreateSolver and CreateSolverByParameters functions which did not exist as separate entitied but combined into a single function CreateSolver(model, custom_settings).

🆕 Changelog
In geo_mechanics_solver.py

  • changed _ConstructBuilderAndSolver(self, param1) ==> _CreateBuilderAndSolver(self)
  • added method _GetLinearSolver(self)

In geomechanics_solvers_wrapper.py

  • splitted operations CreateSolver(model, custom_settings) ==> CreateSolver(model, custom_settings) + CreateSolverByParameters(model, custom_settings, parallelism)

@Rbravo555 Rbravo555 added ROM GeoMechanics Issues related to the GeoMechanicsApplication labels Nov 9, 2023
@Rbravo555 Rbravo555 self-assigned this Nov 9, 2023
Copy link
Contributor

@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.

@Rbravo555 Since I replaced @huhaas in the Kratos GeoMechanics team, I picked up this review while discussing the PR with @avdg81 and @markelov208

Thanks for making the geo solver wrapper consistent with other applications and doing a bit more clean-up! The PR looks clean, we just have a few suggestions to make the changes a bit simpler.


def CreateSolverByParameters(model, custom_settings, parallelism):
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible within the limits of the framework, I would propose to rename this function to CreateSolverBySolverSettings to make it even more clear that the "solver_settings" should be passed to this function

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that your proposed name is more descriptive, as it reflects reliance on solver_settings. However, this function is consistently named CreateSolverByParameters across multiple applications in Kratos, this consistency is crucial for the RomApplication.

Given the implications of renaming a function that is widely used, this change would require consideration and approval by the Technical Committee. I will certainly forward your proposal to them for review.

For the time being, we will maintain the current naming convention to ensure compatibility and consistency across applications. We appreciate your understanding and value your input in this matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for taking it into consideration and explaining the situation, then indeed it is out of scope to change the name in this PR. Also, thank you for implementing the other suggestions, the PR looks ready for merging now from my point of view!

Rbravo555 and others added 3 commits November 20, 2023 11:45
…cs_solvers_wrapper.py

Co-authored-by: Richard Faasse <56549273+rfaasse@users.noreply.github.com>
…cs_solvers_wrapper.py

Co-authored-by: Richard Faasse <56549273+rfaasse@users.noreply.github.com>
Copy link
Contributor

@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.

As mentioned, thank you for processing the suggestions, this PR is ready for merge from my point of view

@Rbravo555 Rbravo555 merged commit 5cbb540 into master Nov 21, 2023
16 checks passed
@Rbravo555 Rbravo555 deleted the geo/rom_pipe_case branch November 21, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GeoMechanics Issues related to the GeoMechanicsApplication ROM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants