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.

Sign up for GitHub

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

Jump to bottom

Half preconditioner, multigrid, and reorder #1713

Open
wants to merge 9 commits into
base: half_factorization
Choose a base branch
from
Open

Half preconditioner, multigrid, and reorder #1713

wants to merge 9 commits into from

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Oct 25, 2024

This PR adds the preconditioner, multigrid, and reorder with half precision support.
move multigrid here because multigrid require direct from #1712 and jacobi here
the reorder is not much, so I include them as well.

@yhmtsai yhmtsai self-assigned this Oct 25, 2024
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:multigrid This is related to multigrid mod:all This touches all Ginkgo modules. labels Oct 25, 2024
@MarcelKoch MarcelKoch self-requested a review October 28, 2024 14:46
MarcelKoch
MarcelKoch requested changes Oct 28, 2024
View reviewed changes
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I would suggest to change the default threshold parameter for MC64.
Additionally, I would prefer to add some explanaitions as to why tests are skipped.

core/preconditioner/jacobi_utils.hpp Outdated Show resolved Hide resolved
dpcpp/test/preconditioner/jacobi_kernels.dp.cpp Outdated Show resolved Hide resolved
reference/test/preconditioner/isai_kernels.cpp Outdated Show resolved Hide resolved
reference/test/reorder/mc64_kernels.cpp Outdated Show resolved Hide resolved
reference/test/reorder/mc64_kernels.cpp Outdated Show resolved Hide resolved
reference/test/reorder/mc64_kernels.cpp Outdated Show resolved Hide resolved
reference/test/reorder/scaled_reordered.cpp Show resolved Hide resolved
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch 3 times, most recently from 240e12f to 3537d26 Compare October 29, 2024 09:17
@yhmtsai yhmtsai force-pushed the half_factorization branch 2 times, most recently from c276034 to bbefde6 Compare October 29, 2024 18:21
@yhmtsai yhmtsai mentioned this pull request Oct 30, 2024
Open
12 tasks
@MarcelKoch MarcelKoch added this to the Ginkgo 1.9.0 milestone Oct 30, 2024
pratikvn
pratikvn requested changes Nov 4, 2024
View reviewed changes
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

Same note here. If we are skipping tests or unable to solve/compute with half with the correct result, then I think we should not support that algorithm with half.

core/distributed/helpers.hpp
const experimental::distributed::Vector<ValueType>,
experimental::distributed::Vector<ValueType>>;
f(dynamic_cast<type*>(linop), std::forward<Args>(args)...);
if constexpr (std::is_same_v<remove_complex<ValueType>, half>) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we dont support half with distributed ? Or will it come in a future PR ?

Copy link
Member Author

Choose a reason for hiding this comment

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

half precision transfer is trivial but the reduce operation and one side accumulation is not trivial.
I have tried adding the custom MPI_Op to support reduce on custom type, which works.
However, one side accumulation does not allow any non-predefined operation.
We will only support half in this case.
Or, use relatively new compiler and MPI standard with its extension, so they can support half natively.
It will be future PR anyways. It might not be in this release, either.

@yhmtsai yhmtsai force-pushed the half_factorization branch 2 times, most recently from 72d9d50 to 88967e6 Compare November 4, 2024 18:15
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch 2 times, most recently from 1e9688c to de5c2a7 Compare November 5, 2024 18:03
@yhmtsai yhmtsai force-pushed the half_factorization branch 2 times, most recently from 257585d to eb14467 Compare November 12, 2024 16:02
MarcelKoch
MarcelKoch approved these changes Nov 13, 2024
View reviewed changes
reference/test/reorder/mc64.cpp Outdated Show resolved Hide resolved
reference/test/reorder/mc64_kernels.cpp Outdated Show resolved Hide resolved
pratikvn
pratikvn approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

Some things on remaining tolerance levels, but otherwise LGTM

@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch 4 times, most recently from 5936c6c to 55be6ed Compare November 14, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcelKoch MarcelKoch MarcelKoch approved these changes

@pratikvn pratikvn pratikvn approved these changes

Assignees

@yhmtsai yhmtsai

Labels
mod:all This touches all Ginkgo modules. reg:testing This is related to testing. type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers
Projects
None yet
Milestone
Ginkgo 1.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@yhmtsai @pratikvn @MarcelKoch @ginkgo-bot