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

batch with half #1716

Merged
merged 3 commits into from
Dec 3, 2024
Merged

batch with half #1716

merged 3 commits into from
Dec 3, 2024

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Oct 29, 2024

This PR enable batch funtionality with half precision

@yhmtsai yhmtsai self-assigned this Oct 29, 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:matrix-format This is related to the Matrix formats mod:all This touches all Ginkgo modules. labels Oct 29, 2024
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from 3537d26 to 046707c Compare October 29, 2024 18:21
@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Oct 29, 2024
@yhmtsai yhmtsai requested a review from a team October 29, 2024 21:49
@yhmtsai yhmtsai mentioned this pull request Oct 30, 2024
Closed
12 tasks
@yhmtsai yhmtsai added this to the Ginkgo 1.9.0 milestone Oct 30, 2024
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from 046707c to 82793ee Compare November 4, 2024 14:24
pratikvn
pratikvn reviewed 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.

Mostly looks good. Some questions on tolerance, but in general we need to really rethink how we set tolerance everywhere in Ginkgo.

reference/test/solver/batch_bicgstab_kernels.cpp
@@ -136,7 +141,7 @@ TYPED_TEST(BatchBicgstab, CanSolveDenseSystem)
using real_type = gko::remove_complex<value_type>;
using Solver = typename TestFixture::solver_type;
using Mtx = typename TestFixture::Mtx;
const real_type tol = 1e-5;
const real_type tol = 1e-4;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe loosen the tolerance only for half ? I think it would be better to leave the tighter tolerance for the other types.

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 think 1e-5 is quite loose for double, so 1e-5 and 1e-4 does not make big difference there.
I can change them only in half precision.
Side note. this condition also means that it is more challenging to half than double/single precision on the machine eps perspective.

reference/test/solver/batch_bicgstab_kernels.cpp
Comment on lines +109 to +116
// There is no guarantee of this condition. We disable this check in
// half.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean there is no guarantee for this condition ? I would assume that they are atleast close.

Copy link
Member Author

Choose a reason for hiding this comment

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

the implicit residual norm can be quite different from the actual residual norm after many iterations.

Copy link
Member

Choose a reason for hiding this comment

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

IMO the check can be removed completely. We can only guarantee that the (implicit) residual will be less than the given tolerance, which is checked above. So this is a quite unrelated check.

Copy link
Member

Choose a reason for hiding this comment

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

I would still suggest to remove this check for all value types.

@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch 2 times, most recently from 1e9688c to de5c2a7 Compare November 5, 2024 18:03
MarcelKoch
MarcelKoch requested changes Nov 6, 2024
View reviewed changes
reference/test/solver/batch_bicgstab_kernels.cpp Show resolved Hide resolved
reference/test/solver/batch_bicgstab_kernels.cpp
Comment on lines +109 to +116
// There is no guarantee of this condition. We disable this check in
// half.
Copy link
Member

Choose a reason for hiding this comment

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

IMO the check can be removed completely. We can only guarantee that the (implicit) residual will be less than the given tolerance, which is checked above. So this is a quite unrelated check.

@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from de5c2a7 to c443777 Compare November 7, 2024 14:40
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from c443777 to 8f47eee Compare November 12, 2024 09:43
@yhmtsai yhmtsai force-pushed the half_batch branch 2 times, most recently from 4995497 to 8e4234d Compare November 12, 2024 16:02
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from 8f47eee to 3329070 Compare November 12, 2024 16:02
@yhmtsai yhmtsai force-pushed the half_batch branch 2 times, most recently from 43bddce to bfc36d0 Compare November 27, 2024 18:42
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch 2 times, most recently from 47a25f9 to 4843cc7 Compare November 28, 2024 18:11
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from 4843cc7 to fdac648 Compare November 28, 2024 19:53
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from fdac648 to f3137db Compare November 29, 2024 15:22
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from f3137db to 6758e9f Compare November 30, 2024 01:30
@yhmtsai yhmtsai force-pushed the half_batch branch 2 times, most recently from aa3f29b to fa2b70d Compare November 30, 2024 18:36
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from 6758e9f to d022134 Compare November 30, 2024 18:36
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Dec 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
31.2% Duplication on New Code (required ≤ 20%)

See analysis details on SonarQube Cloud

@codecov Codecov
Copy link

codecov bot commented Dec 1, 2024

Codecov Report

Attention: Patch coverage is 24.56140% with 43 lines in your changes missing coverage. Please review.

Please upload report for BASE (half_prec_mg_reorder@d022134). Learn more about missing BASE report.

Files with missing lines Patch % Lines
core/device_hooks/common_kernels.inc.cpp 0.00% 21 Missing ⚠️
core/matrix/batch_csr.cpp 0.00% 7 Missing ⚠️
core/matrix/batch_ell.cpp 0.00% 7 Missing ⚠️
core/matrix/batch_dense.cpp 0.00% 6 Missing ⚠️
include/ginkgo/core/log/logger.hpp 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@                   Coverage Diff                   @@
##             half_prec_mg_reorder    #1716   +/-   ##
=======================================================
  Coverage                        ?   89.45%           
=======================================================
  Files                           ?      797           
  Lines                           ?    65819           
  Branches                        ?        0           
=======================================================
  Hits                            ?    58880           
  Misses                          ?     6939           
  Partials                        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch 2 times, most recently from 4a4ac85 to 2b87f17 Compare December 2, 2024 15:36
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Dec 3, 2024
@yhmtsai yhmtsai force-pushed the half_prec_mg_reorder branch from 2b87f17 to 306d06a Compare December 3, 2024 01:28
Base automatically changed from half_prec_mg_reorder to develop December 3, 2024 01:30
@yhmtsai yhmtsai merged commit 368ea30 into develop Dec 3, 2024
6 of 11 checks passed
@yhmtsai yhmtsai deleted the half_batch branch December 3, 2024 01:34
@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

@MarcelKoch MarcelKoch MarcelKoch approved these changes

@pratikvn pratikvn pratikvn approved these changes

Assignees

@yhmtsai yhmtsai

Labels
1:ST:ready-to-merge This PR is ready to merge. 1:ST:run-full-test mod:all This touches all Ginkgo modules. reg:testing This is related to testing. type:matrix-format This is related to the Matrix formats type:preconditioner This is related to the preconditioners 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 @ginkgo-bot @pratikvn @MarcelKoch