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

[Hotfix][Core] Monotonicity preserving with LinearSolversApplication inner solver #11796

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

rubenzorrilla
Copy link
Member

Current module import in the linear solvers Python factory imports the corresponding module from the solver_type value. This means that in the particular case of the monotonicity preserving solver used in combination with an application inner solver it might happen that the corresponding module is not importing when creating the linear solver instance.

Considering that in the vast majority of cases we use a linear solver either form the core or from the LinearSolversApplication I suggest trying to always import the latter in the Python factory to avoid situations as the one I'm describing above. I note that this could be considered a temporary solution as this would no longer be needed if we register the linear solvers to then take the prototypes from the registry.

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

I think this should be done differently.
Instead of importing this always, I recommend to parse the parameters for the inner_solver_settings and import the app if necessary. Reasons:

  • More generic, would also work with solvers from other apps
  • Would also work with the trilinos solvers (required once the Kratos-Matrices are fully supported and thus the Monotonicity preserving solver can be moved to the core)
  • Is not much effort to implement
  • IMO very important: Avoids the import on the cluster, where it can be problematic (intel ...)

@rubenzorrilla
Copy link
Member Author

I think this should be done differently. Instead of importing this always, I recommend to parse the parameters for the inner_solver_settings and import the app if necessary. Reasons:

* More generic, would also work with solvers from other apps

* Would also work with the trilinos solvers (required once the Kratos-Matrices are fully supported and thus the Monotonicity preserving  solver can be moved to the core)

* Is not much effort to implement

* IMO very important: Avoids the import on the cluster, where it can be problematic (intel ...)

Fair enough. I'll do the changes accordingly.

@rubenzorrilla
Copy link
Member Author

@philbucher I think it is ready now 😉

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

great!

I am wondering, would this also work with more nesting levels?

@rubenzorrilla
Copy link
Member Author

great!

I am wondering, would this also work with more nesting levels?

Nope, I assumed one nesting level. Nonetheless, I don't think the recursive nesting is actually a possible use case so I'd leave it as it is. Also note that I also assumed the inner_solver_settings keyword (just for the record).

@rubenzorrilla rubenzorrilla merged commit c252d51 into master Jan 4, 2024
17 checks passed
@rubenzorrilla rubenzorrilla deleted the core/hotfix-monotonicity-preserving-factory branch January 4, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants