Skip to content
This repository has been archived by the owner on Sep 25, 2023. It is now read-only.

[FEA] correlation lags function #459

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

sosaeio
Copy link
Contributor

@sosaeio sosaeio commented Feb 24, 2022

I implemented a cusignal.correlation_lags which works as expected, with notable increase in speed as expected compared to the Scipy function.

I am still working on the pytest and will update that in test_convolution.py once completed soon.

@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@awthomp awthomp added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 24, 2022
@awthomp
Copy link
Member

awthomp commented Feb 24, 2022

ok to test

@awthomp
Copy link
Member

awthomp commented Feb 28, 2022

Looking good, @sosae0. Once we have the tests submitted to the PR, we can merge.

@sosaeio
Copy link
Contributor Author

sosaeio commented Mar 4, 2022

Hi @awthomp, I have a couple of questions.

First, when I run pytest on the existing test_convolution.py, most of the gpu related tests fails and here is an excerpt

FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[valid-symm-5-256]
FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[valid-symm-100-256]
FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[same-fill-5-256]
FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[same-fill-100-256]
FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[same-wrap-5-256]
FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[same-wrap-100-256]
FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[same-symm-5-256]
FAILED test_convolution.py::TestConvolution::TestCorrelate2d::test_correlate2d_gpu[same-symm-100-256]
================ 108 failed, 294 passed, 108 warnings in 43.53s ================
(cusignal-dev) 

Here is a sample error in detail

test_convolution.py:257: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/opt/conda/envs/cusignal-dev/lib/python3.10/site-packages/pytest_benchmark/fixture.py:127: in __call__
    return self._raw(function_to_benchmark, *args, **kwargs)
/opt/conda/envs/cusignal-dev/lib/python3.10/site-packages/pytest_benchmark/fixture.py:173: in _raw
    function_result = function_to_benchmark(*args, **kwargs)
test_convolution.py:225: in gpu_version
    out = cusignal.correlate2d(
../convolution/correlate.py:251: in correlate2d
    out = _convolution_cuda._convolve2d(
../convolution/_convolution_cuda.py:502: in _convolve2d
    out = _convolve2d_gpu(
../convolution/_convolution_cuda.py:378: in _convolve2d_gpu
    _populate_kernel_cache(out.dtype, k_type)
../convolution/_convolution_cuda.py:196: in _populate_kernel_cache
    _cupy_kernel_cache[(str(np_type), k_type)] = _get_function(
../utils/helper_tools.py:71: in _get_function
    return module.get_function(func)
cupy/_core/raw.pyx:485: in cupy._core.raw.RawModule.get_function
    ???
cupy/_core/raw.pyx:96: in cupy._core.raw.RawKernel.kernel.__get__
    ???
cupy/_core/raw.pyx:113: in cupy._core.raw.RawKernel._kernel
    ???
cupy/_util.pyx:67: in cupy._util.memoize.decorator.ret
    ???
cupy/_core/raw.pyx:555: in cupy._core.raw._get_raw_module
    ???
cupy/cuda/function.pyx:262: in cupy.cuda.function.Module.load_file
    ???
cupy_backends/cuda/api/driver.pyx:194: in cupy_backends.cuda.api.driver.moduleLoad
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   cupy_backends.cuda.api.driver.CUDADriverError: CUDA_ERROR_FILE_NOT_FOUND: file not found

cupy_backends/cuda/api/driver.pyx:60: CUDADriverError
=============================== warnings summary ===============================

Secondly, when I run my pytest as a separate file, it works fine but I can't seem to figure out a way to include it with the existing test_convolution.py file. I don't have a benchmark for this yet but here is a pytest that passes for the function I called clags.py for this testing purpose.

import pytest
import cusignal
from clags import correlation_lags
import cupy as cp
from cusignal.test.utils import array_equal


@pytest.mark.parametrize("mode", ["valid", "same", "full"])
@pytest.mark.parametrize("lagging", [True, False])
@pytest.mark.parametrize("int_size", [100, 101, 10000, 10001, 1000000, 1000001])
def test_correlation_lags(mode, lagging, int_size):
    # generate random data
    rng = cp.random.RandomState(0)
    in1 = rng.standard_normal(int_size)
    offset = int(int_size / 10)
    # generate offset version of array to correlate with
    if lagging:
        # y is behind x
        in2 = cp.concatenate([rng.standard_normal(offset), in1])
        expected = -offset
    else:
        # y is ahead of x
        in2 = in1[offset:]
        expected = offset
    # cross correlate, returning lag information
    correlation = cusignal.correlate(in1, in2, mode=mode)
    lags = correlation_lags(in1.size, in2.size, mode=mode)
    # identify the peak
    lag_index = cp.argmax(correlation)
    # Check as expected
    array_equal(lags[lag_index], cp.array(expected))
    # Correlation and lags shape should match
    assert lags.shape == correlation.shape

Also, I am not running this on a local machine so I might have some challenges that I am not even aware of yet. I use the linux terminal provided while using paperspace. I have ensured that the right environments are in place and all dependencies are installed.

Kindly advice. Thanks.

@awthomp
Copy link
Member

awthomp commented Mar 10, 2022

Hey @sosae0!

Sorry for the slow response! I'm on parental leave, and we've been adjusting to our new baby. Let's separate the tests from this PR and get this merged. I can then quickly implement the pytest for it in a successive PR or we can work together to see what's going on with your system.

If you approve, you can remove the 'draft/wip' tag, and LGTM.

@awthomp
Copy link
Member

awthomp commented Mar 10, 2022

@sosae0 --

But to answer your question on where the pytest goes, you should create a new class in test_convolution and stick it at the end. A good example to look at is:

    @pytest.mark.benchmark(group="Correlate")
    @pytest.mark.parametrize("num_samps", [2 ** 7, 2 ** 10 + 1, 2 ** 13])
    @pytest.mark.parametrize("num_taps", [125, 2 ** 8, 2 ** 13])
    @pytest.mark.parametrize("mode", ["full", "valid", "same"])
    @pytest.mark.parametrize("method", ["direct", "fft", "auto"])
    class TestCorrelate:
        def cpu_version(self, sig, num_taps, mode, method):
            return signal.correlate(sig, num_taps, mode=mode, method=method)

        def gpu_version(self, sig, num_taps, mode, method):
            with cp.cuda.Stream.null:
                out = cusignal.correlate(
                    sig, num_taps, mode=mode, method=method
                )
            cp.cuda.Stream.null.synchronize()
            return out

        @pytest.mark.cpu
        def test_correlate1d_cpu(
            self,
            rand_data_gen,
            benchmark,
            num_samps,
            num_taps,
            mode,
            method,
        ):
            cpu_sig, _ = rand_data_gen(num_samps, 1)
            cpu_filt, _ = rand_data_gen(num_taps, 1)
            benchmark(self.cpu_version, cpu_sig, cpu_filt, mode, method)

        def test_correlate1d_gpu(
            self,
            rand_data_gen,
            gpubenchmark,
            num_samps,
            num_taps,
            mode,
            method,
        ):

            cpu_sig, gpu_sig = rand_data_gen(num_samps, 1)
            cpu_filt, gpu_filt = rand_data_gen(num_taps, 1)
            output = gpubenchmark(
                self.gpu_version,
                gpu_sig,
                gpu_filt,
                mode,
                method,
            )

            key = self.cpu_version(cpu_sig, cpu_filt, mode, method)
            array_equal(output, key)

You'll need to do something like:

    @pytest.mark.benchmark(group="CorrelationLags")
    @pytest.mark.parametrize("in_len1", [100, 1000, 100000])
    @pytest.mark.parametrize("in_len2",[100, 1000, 100000])
    @pytest.mark.parametrize("mode", ["full", "valid", "same"])
    class TestCorrelationLags:
        def cpu_version(self, sig, num_taps, mode, method):
            return signal.correlation_lags(in_len1, in_len2, mode=mode)

        def gpu_version(self, sig, num_taps, mode, method):
            with cp.cuda.Stream.null:
                out = cusignal.correlation_lags(
                    in_len1, in_len2, mode=mode
                )
            cp.cuda.Stream.null.synchronize()
            return out

And the rest should be relatively straightforward. We can help!

@mnicely, in particularly, can help on getting the pytests up and running. That said, I still don't see anything in the PR that's going to throw errors on the tests -- hence, why I want to get this merged and worry about the test in a separate PR.

@sosaeio
Copy link
Contributor Author

sosaeio commented Mar 10, 2022

Hi @awthomp , thanks for the feedback and congratulations ones again. TBH I was even expecting a later response but you showed up earlier :).

That updated pytest looks good. I will create a separate PR for that and merge this.

@sosaeio sosaeio changed the title [WIP] correlation lags function [FEA] correlation lags function Mar 10, 2022
@sosaeio sosaeio marked this pull request as ready for review March 10, 2022 15:33
@sosaeio sosaeio requested a review from a team as a code owner March 10, 2022 15:33
Copy link
Member

@awthomp awthomp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the great contribution, @sosae0 !

@awthomp
Copy link
Member

awthomp commented Mar 10, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 40a5e76 into rapidsai:branch-22.04 Mar 10, 2022
@sosaeio
Copy link
Contributor Author

sosaeio commented Mar 10, 2022

LGTM. Thanks for the great contribution, @sosae0 !

Thanks for creating this great project. Looking forward to doing more with it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants