Skip to content

Feat/conv 1 d #1907

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

Open
wants to merge 6 commits into
base: conv
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions core/matrix/conv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ void Conv<ValueType>::apply_impl(const LinOp* b, LinOp* x) const
{
precision_dispatch_real_complex<ValueType>(
[this](auto dense_b, auto dense_x) {
this->get_executor()->run(conv::make_conv(this, dense_b, dense_x));
this->get_executor()->run(
conv::make_conv(kernel_, dense_b, dense_x));
},
b, x);
}
Expand All @@ -50,7 +51,19 @@ template <typename ValueType>
void Conv<ValueType>::validate_application_parameters(const LinOp* b,
const LinOp* x) const
{
// implement dimension validation throw DimensionMismatch when it is wrong
using gko::detail::get_size;
const auto b_rows = get_size(b)[0];
const auto x_rows = get_size(x)[0];
const auto kernel_len = kernel_.get_size();

if (x_rows != b_rows + kernel_len - 1) {
throw DimensionMismatch(__FILE__, __LINE__, __func__, "x", x_rows, 1,
"b + kernel - 1", b_rows + kernel_len - 1, 1,
"x must have size = b + kernel - 1");
}


GKO_ASSERT_EQUAL_COLS(b, x);
}


Expand Down
2 changes: 1 addition & 1 deletion core/matrix/conv_kernels.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace kernels {

#define GKO_DECLARE_CONV_KERNEL(ValueType) \
void conv(std::shared_ptr<const DefaultExecutor> exec, \
const matrix::Conv<ValueType>* kernel, \
const array<ValueType>& kernel, \
const matrix::Dense<ValueType>* b, matrix::Dense<ValueType>* x)


Expand Down
4 changes: 2 additions & 2 deletions omp/matrix/conv_kernels.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ namespace conv {

template <typename ValueType>
void conv(std::shared_ptr<const DefaultExecutor> exec,
const matrix::Conv<ValueType>* kernel,
const matrix::Dense<ValueType>* b, matrix::Dense<ValueType>* x)
const array<ValueType>& kernel, const matrix::Dense<ValueType>* b,
matrix::Dense<ValueType>* x)
{
GKO_NOT_IMPLEMENTED;
}
Expand Down
21 changes: 18 additions & 3 deletions reference/matrix/conv_kernels.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,25 @@ namespace conv {

template <typename ValueType>
void conv(std::shared_ptr<const DefaultExecutor> exec,
const matrix::Conv<ValueType>* kernel,
const matrix::Dense<ValueType>* b, matrix::Dense<ValueType>* x)
const array<ValueType>& kernel, const matrix::Dense<ValueType>* b,
matrix::Dense<ValueType>* x)
{
GKO_NOT_IMPLEMENTED;
const auto b_size = b->get_size(); // (N, 1)
const auto x_size = x->get_size(); // (N + K - 1, 1)
const auto kernel_size = kernel.get_size(); // K
const auto* kernel_ptr = kernel.get_const_data(); // pointer to kernel data

for (int i = 0; i < x_size[0]; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

it seems not to be correct.
your formula is x[0] = kernel[0] * b[0];
but it should be x[i] = sum_k kernel[k] * b[i+k], right?

Copy link
Collaborator Author

@keshvituteja keshvituteja Aug 14, 2025

Choose a reason for hiding this comment

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

I updated it according to what torch does, it should work fine now c629e5b

Copy link
Member

Choose a reason for hiding this comment

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

you need to use size_t to deal from the dense size

Copy link
Collaborator Author

@keshvituteja keshvituteja Aug 14, 2025

Choose a reason for hiding this comment

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

I changed it to size_t in 630a24d, does size_t allow negative values? I am currently using std::ptrdiff_t for negative values

Copy link
Member

Choose a reason for hiding this comment

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

I tend to go by the rule of thumb: if you ever need to use a subtraction (even if you think the values will never become negative), you should use a signed type, so I would agree with using int64 or ptrdiff_t

ValueType sum = zero<ValueType>();
for (int j = 0; j < kernel_size; ++j) {
int b_idx = i - j;
if (b_idx >= 0 && b_idx < b_size[0]) {
sum +=
kernel_ptr[j] * b->at(b_idx, 0); // direct pointer access
}
}
x->at(i, 0) = sum;
}
}

GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE(GKO_DECLARE_CONV_KERNEL);
Expand Down
Loading