feat : support for conv3D forward

Context

issue : #38 (closed), aidge#252 (closed)

Added :

• Conv3D
• ConvTranspose3D
• Pad3D
• PaddedConv3D
• PaddedConvTranspose3D

Enhancements

Conv1D & Conv2D backward functions have been optimized : indexing is now done by only incrementing the index counters without using multiplication.

Before

for (DimSize_t kX = 0; kX < kDim[0]; ++kX) {
    kOffsets[2] = kX * kStrides[2] + kOffsets[1];
    iDilKernelOffsets[0] = kX * dilation[0] * iStrides[2];

    // DOING STUFF ...........
}

After

iDilKernelOffsets[0] = 0;
kOffsets[2] = kOffsets[1];
for (DimSize_t kX = 0; kX < kDims[0]; ++kX,
               kOffsets[2] += kStrides[2],
               iDilKernelOffsets[0] += dilation[0] *
                                       iStrides[2]) {
    // DOING STUFF ...........
}

NOTE : This implies a bit of unconventionnal over-crowding the for loops. That is usually written as :

for (DimSize_t kX = 0; kX < kDims[0]; ++kX) {
    // DOING STUFF ...........
    kOffsets[2] += kStrides[2];
    iDilKernelOffsets[0] += dilation[0] * iStrides[2];
}

but since the the //DOING STUFF....... usually contains 3+ nested loops, this would quickly be un-readable. Hence I chose to use the 1st "overcrowded for loop" syntax.

added Missing 💨 TopicOperator labels

assigned to @gregkub

requested review from @hrouis

requested review from @pineapple

marked this merge request as draft

changed the description

This implies a bit of unconventionnal over-crowding the for loops :

I don't find your 1st syntax more clear as it mix the logic of the index iteration and update of variables.

I just think it is an unconvential way to do for a reason. I would require a change to the 2nd syntax.

Ok so before I modify all functions :

• here is the original code I wrote
• Here is the same function modified accordingly to your requirements in the latest commit.

Could you confirm this is what you want ? I'll do the modifications after that.

thanks

changed the description

added 5 commits

• 8884f722 - feat: [ADD] conv3D
• 98d81e0f - feat : speed up conv1Dbackward
• ee61759d - feat : speed up conv2D backward execution
• 5adf740b - feat : ConvTranspose 3D
• 19efb308 - fix : conv2Dbackward weight in and out channels were inverted

Compare with previous version

IMO, this kind of loop indexing optimizations are not worth it, for several reasons:

• It makes the code much less readable, as it become much harder to understand the indexing mechanism;
• It will need to be rewritten anyway if one decides to parallelize the loop (for example with OpenMP);
• I think it will change absolutely nothing in terms of performances: it is most certainly already optimized by the compiler.

added 5 commits

• 1511c7d5 - feat: [ADD] conv3D
• 8edb12a5 - feat : speedup Conv1D Backward execution
• 7a994816 - feat : speed up Conv2D Backward execution
• b55eaf41 - feat : ConvTranspose 3D
• b2a4a2e5 - fix : conv2Dbackward weight in and out channels were inverted

Compare with previous version

added 1 commit

• 6ab1f9d4 - chore : rewrote 1 function in alternative syntax as per @cmoineau request for test

Compare with previous version

Also with the format you adopted conv test is unreadable (14k lines ...).

Either a different test format is required or we need to store the tensors in another way since this will create a file that is way too big.

For example such a regular array: https://gitlab.eclipse.org/eclipse/aidge/aidge_backend_cpu/-/blame/8b09d81fca5dcc21abdda2d45cbb76c26184d395/unit_tests/operator/Test_ConvImpl.cpp?page=6#L5972 should be done using std::iota instead of writting 4000 + lines ...

We talked with @pineapple and I think this unit test really higlight the limitation of the current way we have of testing Operators.

I would be in favor of dropping this file (and the commit associated in the hisotry) since it will add almost 1MB to the code base. And to move to test using benchmark tools to compare with ONNRT implementations.

What I propose is to merge this without unit test and add an issue for creating unit test using benchmark tools and mark the untested operator of this MR to be added in these tests.

@cmoineau I cleaned up the tests to reduce the file size 10k lines, I also cleaned up previous 1/2D backward tests for conv in this commit : !160 (f4235226)

I am not authorized to but push on dev but this commit could be fused with c0dbb037 for cleaner history

requested review from @cmoineau and removed review request for @hrouis

added 2 commits

• d364a2e2 - feat: [ADD] conv3D
• d133984c - feat : ConvTranspose 3D

Compare with previous version

added 3 commits

• 812b59da - feat: [ADD] conv3D
• f992a4d3 - feat : ConvTranspose 3D
• 885f95ef - feat : [ADD] Padding3D

Compare with previous version

added 1 commit

• 4472701f - feat : [ADD] Padding3D

Compare with previous version

added 1 commit

• 80def0ca - feat : [ADD] Padding3D

Compare with previous version

added 1 commit

• cda0979c - feat : [ADD] Padding3D

Compare with previous version

added 2 commits

• 66ca05d2 - feat : [ADD] Padding3D
• dcba7319 - feat : pad3D

Compare with previous version