Feat/conv 1 d #1907
Feat/conv 1 d #1907
Conversation
ginkgo-bot
added
reg:build
|
I'm wondering, should this be called a convolution, or rather something like "stencil"? In the context of finite difference methods, convolution may be overly specific, but the same kernel is still useful. |
We are planning to use it in context of Machine Learning (through pyGinkgo), so, we decided to name it 'Convolution' for now. I/We are open to the naming scheme |
|
could you rebase your branch on origin/conv first to reduce the file changes? |
keshvituteja
force-pushed
the
feat/conv_1D
branch
from
7e454b8 to
28fd91e
Compare
|
The base commit seems incorrect, this should be based on/merged into |
keshvituteja
force-pushed
the
feat/conv_1D
branch
from
64d0f9d to
a59f27c
Compare
|
@upsj It is not ready for merging into develop. |
reference/matrix/conv_kernels.cpp
Outdated
| 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I updated it according to what torch does, it should work fine now c629e5b
reference/matrix/conv_kernels.cpp
Outdated
| 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) { |
There was a problem hiding this comment.
you need to use size_t to deal from the dense size
There was a problem hiding this comment.
I changed it to size_t in 630a24d, does size_t allow negative values? I am currently using std::ptrdiff_t for negative values
There was a problem hiding this comment.
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
Supports single rhs currently, added an assert statement to ensure that Co-authored-by: Yu-Hsiang M. Tsai <19565938+yhmtsai@users.noreply.github.com>
core/matrix/conv.cpp
Outdated
| throw DimensionMismatch( | ||
| __FILE__, __LINE__, __func__, "x", x_rows, 1, | ||
| "(b + 2*padding - kernel)/stride + 1", | ||
| (b_rows + 2 * 2 - kernel_len) / 1 + 1, 1, |
There was a problem hiding this comment.
| (b_rows + 2 * 2 - kernel_len) / 1 + 1, 1, | |
| (b_rows + 2 * 0 - kernel_len) / 1 + 1, 1, |
shouldn't padding be zero now?
There was a problem hiding this comment.
padding equal 2 in your example such that it become numpy result.
IMO, we should follow the default from torch not numpy.
also, if it needs to fit numpy, it should be something like ((x-1)*stride + kernel - b)/2
There was a problem hiding this comment.
The example also matches the torch result. We can change the padding inside the conv_kernels.cpp. It's result would be same as this function:
torch.nn.functional.conv1d(x, w, bias=None, stride=1, padding=2)
We can accept padding in arguments but currently we don't do that, so i hard coded it to be 2. The function also works for zero-padding. For zero padding, it should be similar to:
torch.nn.functional.conv1d(x, w, bias=None, stride=1, padding=0)
There was a problem hiding this comment.
I changed it to zero but it will work if change it to any other padding as well
| int padding = 2; | ||
| int output_length = (x_size[0] + 2 * padding - kernel_size) / stride + 1; | ||
|
|
||
| for (gko::size_type i = 0; i < x_size[0]; ++i) { |
There was a problem hiding this comment.
because you are using substraction later, so I will say you can use gko::int64 directly as Tobias mentioned
There was a problem hiding this comment.
also I think start with output will be easier than with input
reference/matrix/conv_kernels.cpp
Outdated
| static_cast<std::ptrdiff_t>( | ||
| j); // calculate the index in b's row based on the current |
There was a problem hiding this comment.
| static_cast<std::ptrdiff_t>( | |
| j); // calculate the index in b's row based on the current | |
| static_cast<std::ptrdiff_t>( | |
| j) * stride; // calculate the index in b's row based on the current |
There was a problem hiding this comment.
We might not need the multplication with stride here since we already multiply with stride while calculatingstart in line 41:
gko::int64 start =static_cast<gko::int64>(i * stride) - padding;
Added an implementation of the 1D convolution with stride 1.