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

solver (mostly krylov solver) and the residual norm with half #1711

Merged
merged 7 commits into from
Dec 3, 2024
Merged

solver (mostly krylov solver) and the residual norm with half #1711

merged 7 commits into from
Dec 3, 2024

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Oct 25, 2024 •
edited
Loading

This PR enable the solvers and the residual norm with half. It also includes the corresponding config

@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Oct 25, 2024
@yhmtsai yhmtsai self-assigned this Oct 25, 2024
@yhmtsai yhmtsai added this to the Ginkgo 1.9.0 milestone Oct 25, 2024
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. type:solver This is related to the solvers type:stopping-criteria This is related to the stopping criteria mod:all This touches all Ginkgo modules. labels Oct 25, 2024
@yhmtsai yhmtsai requested a review from a team October 25, 2024 15:37
pratikvn
pratikvn reviewed Oct 25, 2024
View reviewed changes
common/cuda_hip/solver/idr_kernels.cpp
@@ -555,7 +555,8 @@ void initialize(std::shared_ptr<const DefaultExecutor> exec,
orthonormalize_subspace_vectors(exec, subspace_vectors);
}

GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE(GKO_DECLARE_IDR_INITIALIZE_KERNEL);
GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE_WITH_HALF(
Copy link
Member

Choose a reason for hiding this comment

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

A general question: Why do we need another macro _WITH_HALF ? Is it not possible to just have half within the first macro ?

Copy link
Member Author

@yhmtsai yhmtsai Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is possible but it will result it in #1257 because we need to finish everything all at once.

Copy link
Member Author

Choose a reason for hiding this comment

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

#1257 is to make everything work with half first and then disable some we can not support now.
The recent prs try to make the half support block by block. hopefully, they are easier for review.
To do it block by block, I need to create many list with half additionally unfortunately to reduce the touching other stuff.

@MarcelKoch MarcelKoch self-requested a review October 28, 2024 10:52
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.

LGTM. Can you clarify the testing issues with IDR?

core/test/solver/gcr.cpp Outdated Show resolved Hide resolved
reference/test/solver/idr_kernels.cpp Outdated Show resolved Hide resolved
@yhmtsai yhmtsai force-pushed the half_matrix branch 2 times, most recently from f53438a to 5b22346 Compare October 28, 2024 17:19
@yhmtsai yhmtsai mentioned this pull request Oct 30, 2024
Closed
12 tasks
@MarcelKoch MarcelKoch requested a review from pratikvn October 30, 2024 14:07
pratikvn
pratikvn approved these 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.

I think it makes sense to remove half support for solvers which do not converge with half ? (IDR, GCR ?) Otherwise, looks good to me.

core/config/config_helper.hpp
@@ -200,7 +200,9 @@ get_value(const pnode& config)
* This is specialization for floating point type
*/
template <typename ValueType>
inline std::enable_if_t<std::is_floating_point<ValueType>::value, ValueType>
inline std::enable_if_t<std::is_floating_point<ValueType>::value ||
Copy link
Member

Choose a reason for hiding this comment

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

I think this structure will be useful everywhere. Maybe it would be nice to have our own is_floating_point which includes half ? So all you will need to do is gko::is_floating_point<ValueType>::value ?

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 only use it here because others usually need to be writen separately

@yhmtsai yhmtsai requested a review from MarcelKoch November 5, 2024 17:45
MarcelKoch
MarcelKoch requested changes Nov 6, 2024
View reviewed changes
core/solver/idr.cpp Outdated
Comment on lines 286 to 287
// if norm(t) is zero
// omega = 0
// end if
Copy link
Member

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 moved above and merged with the other definition of omega

Copy link
Member Author

Choose a reason for hiding this comment

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

have moved

reference/test/solver/gcr_kernels.cpp Outdated
@@ -330,7 +332,7 @@ TYPED_TEST(Gcr, SolvesStencilSystemUsingAdvancedApplyMixed)
solver->apply(alpha.get(), b.get(), beta.get(), x.get());

GKO_ASSERT_MTX_NEAR(x, l({1.5, 5.0, 2.0}),
(r_mixed<value_type, TypeParam>()) * 1e1);
(r_mixed<value_type, TypeParam>()) * 1e2);
Copy link
Member

Choose a reason for hiding this comment

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

It is enough to scale the old tolerance with alpha. We should actually do this for all solver advanced apply tests.

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 also check the other tol changes in this file. they are not needed at least in cuda10.2 image. need to check the other ci job

@yhmtsai yhmtsai force-pushed the half_matrix branch 2 times, most recently from b4470ad to ebb6468 Compare November 27, 2024 18:42
@yhmtsai yhmtsai force-pushed the half_solver branch 2 times, most recently from a09d80a to 8f64d67 Compare November 28, 2024 18:11
@yhmtsai yhmtsai force-pushed the half_matrix branch 2 times, most recently from ff85d00 to 2aa59e2 Compare November 30, 2024 01:30
@yhmtsai yhmtsai force-pushed the half_solver branch 2 times, most recently from 3a98c11 to ac216bc Compare November 30, 2024 18:36
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test and removed 1:ST:ready-for-review This PR is ready for review labels Dec 3, 2024
Base automatically changed from half_matrix to develop December 3, 2024 01:11
@yhmtsai yhmtsai merged commit 066ca3c into develop Dec 3, 2024
8 of 11 checks passed
@yhmtsai yhmtsai deleted the half_solver branch December 3, 2024 01:22
@ginkgo-bot
Copy link
Member

Error: PR already merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pratikvn pratikvn pratikvn approved these changes

@MarcelKoch MarcelKoch MarcelKoch approved these changes

Assignees

@yhmtsai yhmtsai

Labels
1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test mod:all This touches all Ginkgo modules. reg:testing This is related to testing. type:solver This is related to the solvers type:stopping-criteria This is related to the stopping criteria
Projects
None yet
Milestone
Ginkgo 1.9.0
Development

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