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

Ginkgo config solver #272

Open
wants to merge 10 commits into
base: stack/implicitOperators
Choose a base branch
from

Conversation

MarcelKoch
Copy link
Collaborator

Motivation

Replaces the Ginkgo solvers, by a single solver which is configured through a ginkgo property tree.

This allows to use any Ginkgo solver and preconditioner combination that is available.

TODO: check how the value type in the property tree should be handled.

@MarcelKoch MarcelKoch self-assigned this Mar 5, 2025
Copy link

github-actions bot commented Mar 5, 2025

Deployed test documentation to https://exasim-project.com/NeoFOAM/Build_PR_272

Copy link
Collaborator

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

do you mean something like type_descriptor for valuetype?

Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>
@MarcelKoch MarcelKoch force-pushed the feat/ginkgo-config-solver branch from 398638c to 0c6cbe1 Compare March 6, 2025 08:23
@@ -132,8 +132,7 @@ void solve(
using ValueType = typename FieldType::ElementType;
auto ls = ginkgoMatrix(exp, solution);


NeoFOAM::la::ginkgo::BiCGStab<ValueType> solver(solution.exec(), fvSolution);
NeoFOAM::la::ginkgo::Solver<ValueType> solver(solution.exec(), fvSolution);
Copy link
Collaborator

Choose a reason for hiding this comment

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

line 116-119 format seems to be incorrect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

clang format seems to be fine with it.

Comment on lines 32 to 39
if (auto node = parseAny(int {}))
{
return node;
}
if (auto node = parseAny((unsigned int) {}))
{
return node;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

any other integer format from Dictionary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly, but it doesn't make much sense right now to check for every possibility, since the dictionary is too unconstrained.


#if NF_WITH_GINKGO

bool operator==(const gko::config::pnode& a, const gko::config::pnode& b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I create a pr in ginkgo-project/ginkgo#1801

@@ -12,12 +12,14 @@
#if NF_WITH_GINKGO

template<typename ExecSpace>
bool isNotKokkosThreads(ExecSpace ex)
bool isNotKokkosThreads([[maybe_unused]] ExecSpace ex)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bool isNotKokkosThreads([[maybe_unused]] ExecSpace ex)
bool isNotKokkosThreads(ExecSpace)

it should also eliminate the warning by deleting the variable name.
but I am okay with the keywords

- fixes tests
- fixes formatting

Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>
This is necessary to prevent clashes with existing cmake targets from third-party libraries build through CPM.
Copy link
Collaborator

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

I have created #274 for the supported type concern in Dictionary. Other that that, LGTM

exec, gko::dim<2> {nrows, nrows}, valuesView, colIdxView, rowPtrView
return gko::share(gko::matrix::Csr<ValueType, IndexType>::create(
exec,
gko::dim<2> {nrows, nrows},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: nrows from mtx.nRows() not from rhs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now the matrices are assumed to be square. I think this will also stay like this for quite a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants