Feature/920: Implemented 2D convolution

[shahpratham]

Description

Implemented 2D convolution with distributed kernel support.

Issue resolved: #920

skip ci

[ghost]

👇 Click on the image for a new way to code review

Legend

[ClaudiaComito]

Is this still valid?

[shahpratham]

yes, when a is of dimension (4, 15) and v is of dimension (5, 10)

[krajsek]

Maybe a more precise Error message: "Filter size must not be larger in one dimension and smaller in the other" as the user might not be aware of the swapping in case of the filter size beeing larger in both dimensions

[ClaudiaComito]

There seems to be a new problem in the CI:

from . import pypocketfft as pfft
E   ImportError: /opt/conda/lib/python3.10/site-packages/torch/lib/../../../.././libstdc++.so.6: version `GLIBCXX_3.4.30' not found (required by /opt/conda/lib/python3.10/site-packages/scipy/fft/_pocketfft/pypocketfft.cpython-310-x86_64-linux-gnu.so)

The problem is discussed here among many other places: https://stackoverflow.com/questions/72540359/glibcxx-3-4-30-not-found-for-librosa-in-conda-virtual-environment-after-tryin
I'm not sure what the best way to handle this is, within our CI images.

It's the cuda one only. Conda installs some older packages and outputs some warning there: WARNING conda.models.version:get_matcher(556): Using .* with relational operator is superfluous and deprecated and will be removed in a future version of conda. Your spec was 1.7.1., but conda is ignoring the . and treating it as 1.7.1

I don't understand @mtar . What spec?

Thanks for looking into this btw.

[mtar]

The issue happens only on the Cuda Runner, likely because of outdated libraries installed by conda. The failure on the amd runner is caused by a device mismatch in ht.convolve[2d] instead.

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[mtar]

@ClaudiaComito
Tests on cuda runner working; now same errors as on amd runner

[github-actions]

This pull request is stale because it has been open for 60 days with no activity.

[github-actions]

Thank you for the PR!

[ClaudiaComito]

I cannot reproduce the CUDA problems locally, but even on CPU I run into different problems with mismatching split axes (and local shapes):

======================================================================
======================================================================
ERROR: test_convolve2d (heat.core.tests.test_signal.TestSignal)
ERROR: test_convolve2d (heat.core.tests.test_signal.TestSignal)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/c.comito/devel/local/heat/heat/core/tests/test_signal.py", line 226, in test_convolve2d
    conv = ht.convolve2d(signal, dis_kernel_odd, mode=mode)
  File "/Users/c.comito/devel/local/heat/heat/core/signal.py", line 635, in convolve2d
    signal_filtered += global_signal_filtered[start_idx : start_idx + gshape[0]]
  File "/Users/c.comito/devel/local/heat/heat/core/arithmetics.py", line 188, in add_
    return _operations.__binary_op(wrap_add_, t1, t2, out=t1)
  File "/Users/c.comito/devel/local/heat/heat/core/_operations.py", line 197, in __binary_op
    sanitation.sanitize_out(out, output_shape, output_split, output_device, output_comm)
  File "/Users/c.comito/devel/local/heat/heat/core/sanitation.py", line 300, in sanitize_out
    raise ValueError(f"Expecting output buffer of split {output_split}, got {out.split}")
ValueError: Expecting output buffer of split 0, got None

(tests run on 2 processes).

[ClaudiaComito]

I'm setting this PR to draft and marking it for next Monday's discussion. My thoughts so far (fed from discussions with @mrfh92 in the past):

• code still needs significant debugging
• fully distributed (signal and kernel) scaling behaviour unclear
• fully distributed via FFT is being implemented (Distributed Convolution via FFT #1291 )

Options for this PR:

1. fix bugs and merge independent of scaling behaviour (needs work)
2. throw out distributed-kernel implementation, keep distributed-signal option only (needs work but will cover most use cases)
3. do not merge
4. ...??

What do you think @mrfh92 @krajsek @mtar ?

[github-actions]

This pull request is stale because it has been open for 60 days with no activity.