-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add Sor kernels #1634
Add Sor kernels #1634
Conversation
0d29fae
to
1f1ef07
Compare
a94cab2
to
7c59123
Compare
a30fee7
to
79ed6d7
Compare
namespace factorization { | ||
namespace helpers { | ||
|
||
using namespace ::gko::factorization; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does not help anything below, so I would rather remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this more reasonable, I also removed the factorization
namespace in the core file. Otherwise I would have had to use ::gko::factorization::
everywhere instead of just helpers::
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I thought they are in the same namespace, but one is under kernel namespace and the other is under gko directly.
then the original one is better as it does not give any ambiguous function.
although it can only declare the two class you need, it's a bit annoying with the template.
namespace helpers { | ||
|
||
|
||
using namespace ::gko::factorization; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here and for the others.
@@ -0,0 +1,67 @@ | |||
// SPDX-FileCopyrightText: 2017 - 2024 The Ginkgo authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the file can go into unified one but you need to do dispatch on the header of factorization_helpers.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may need to use GKO_KERNEL
for the lambda function.
Maybe also for the actual kernel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this complicates our unified setup again, and it's not using the unified kernels as intended.
We use the unified kernels, when the kernel implementation are identical, which is not the case here. I think it's close, but it would require some adjustments of the factorization kernels. If that's feasible, I would prefer to do it in another PR.
} // namespace factorization | ||
} // namespace GKO_DEVICE_NAMESPACE | ||
} // namespace kernels | ||
} // namespace gko |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} // namespace gko | |
} // namespace gko | |
nit
namespace factorization { | ||
namespace helpers { | ||
|
||
using namespace ::gko::factorization; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I thought they are in the same namespace, but one is under kernel namespace and the other is under gko directly.
then the original one is better as it does not give any ambiguous function.
although it can only declare the two class you need, it's a bit annoying with the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I need to take my previous comment about the using namespace back because I thought they are in the same namespace, which is wrong.
edb44d4
to
40a0e9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
27f0e04
to
de40cea
Compare
a961299
to
d2f4b0a
Compare
220cb98
to
1cb476a
Compare
9f4c666
to
04783d0
Compare
04783d0
to
43c7810
Compare
43c7810
to
2271b5f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1634 +/- ##
===========================================
- Coverage 90.55% 90.09% -0.47%
===========================================
Files 776 779 +3
Lines 63205 63336 +131
===========================================
- Hits 57237 57061 -176
- Misses 5968 6275 +307 ☔ View full report in Codecov by Sentry. |
9b8363a
to
c2aad74
Compare
Quality Gate failedFailed conditions |
This PR adds the device kernels for #1633.