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

Register Single Precision Linear Solvers #13087

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

matekelemen
Copy link
Contributor

@matekelemen matekelemen commented Feb 4, 2025

This PR is a follow-up on #13079.

Changelog

  • Fix some inconsistencies in the implementation of core linear solvers. These inconsistencies usually mean:
    • hard-coding double as the value type in some functions.
    • using the dense space's value type in places where the sparse space is used.
    • moving the definition of some function templates to source files without providing explicit instantiations for value types other than double.
  • Instantiate and register single precision versions of core linear solvers.
  • Extend Serializer to support std::complex<float>.

@matekelemen matekelemen self-assigned this Feb 4, 2025
@matekelemen matekelemen requested a review from a team as a code owner February 4, 2025 11:10
Copy link
Member

@loumalouomega loumalouomega left a comment

Choose a reason for hiding this comment

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

LGTM

@matekelemen
Copy link
Contributor Author

matekelemen commented Feb 4, 2025

Looks like every compiler is happy now. @loumalouomega can you please take a look again?

I'd also appreciate if someone from the @KratosMultiphysics/technical-committee too could take a quick peek.

// using LinearSolverType = LinearSolver<SpaceType, LocalSpaceType>;
// using IterativeSolverType = IterativeSolver<SpaceType, LocalSpaceType>;
using CGSolverType = CGSolver<SpaceType, LocalSpaceType>;
using SpaceType = TUblasSparseSpace<TSparseDataType>;
Copy link
Member

Choose a reason for hiding this comment

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

BTW, we should eventually replace the linear solver factory with the register @KratosMultiphysics/technical-committee I don't know if a good moment to do that

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 was thinking about that too, but other parts of Kratos still expect linear solvers to be in KratosComponents and I didn't want to change too many unrelated things at once.

Copy link
Member

Choose a reason for hiding this comment

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

A priori we can define in both database and add a warning in the factory, opinion @KratosMultiphysics/technical-committee ?

@@ -541,6 +541,7 @@ class KRATOS_API(KRATOS_CORE) Serializer
KRATOS_SERIALIZATION_DIRECT_LOAD(std::size_t)
#endif
KRATOS_SERIALIZATION_DIRECT_LOAD(std::complex<double>)
Copy link
Member

Choose a reason for hiding this comment

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

We should remove those evil tabs

Copy link
Member

@loumalouomega loumalouomega left a comment

Choose a reason for hiding this comment

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

For me ok, I just added 2 comments

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.

2 participants