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

adding complex parameter to chirp and additional tests #450

Merged
merged 6 commits into from
Jan 28, 2022

Conversation

mnicely
Copy link
Contributor

@mnicely mnicely commented Jan 20, 2022

This PR add parity with MATLAB chirp, specifically complex output.

Adding the following parameter (dtype=cp.float64)
Example input (w = cusignal.chirp(t, cf-100, t[-1], cf+100, dtype=cp.complex128))

Perf on original

test/test_waveforms.py ........                                                                                                                                                                                                                                                               [100%]


-------------- benchmark 'Chirp': 8 tests --------------
Name (time in us)                         Mean          
--------------------------------------------------------
test_chirp_gpu[quad-10-1-6-16384]      61.1752 (1.0)    
test_chirp_gpu[lin-10-1-6-16384]       61.2585 (1.00)   
test_chirp_gpu[hyp-10-1-6-16384]       63.2589 (1.03)   
test_chirp_gpu[log-10-1-6-16384]       70.5225 (1.15)   
test_chirp_cpu[lin-10-1-6-16384]      179.8704 (2.94)   
test_chirp_cpu[hyp-10-1-6-16384]      234.5341 (3.83)   
test_chirp_cpu[log-10-1-6-16384]      394.2949 (6.45)   
test_chirp_cpu[quad-10-1-6-16384]     403.2681 (6.59)   
--------------------------------------------------------

Perf on new
Notice:

  • float64 is about 4x faster... There was a bug in test suite causing memory copy
----------------- benchmark 'Chirp': 16 tests ------------------
Name (time in us)                                 Mean          
----------------------------------------------------------------
test_chirp_gpu[lin-10-1-6-16384-real]          17.7974 (1.0)    
test_chirp_gpu[quad-10-1-6-16384-complex]      18.9554 (1.07)   
test_chirp_gpu[quad-10-1-6-16384-real]         19.1889 (1.08)   
test_chirp_gpu[hyp-10-1-6-16384-real]          19.7320 (1.11)   
test_chirp_gpu[hyp-10-1-6-16384-complex]       19.7500 (1.11)   
test_chirp_gpu[lin-10-1-6-16384-complex]       22.5316 (1.27)   
test_chirp_gpu[log-10-1-6-16384-complex]       26.4949 (1.49)   
test_chirp_gpu[log-10-1-6-16384-real]          28.1316 (1.58)   
test_chirp_cpu[lin-10-1-6-16384-real]         182.4054 (10.25)  
test_chirp_cpu[hyp-10-1-6-16384-complex]      232.2149 (13.05)  
test_chirp_cpu[hyp-10-1-6-16384-real]         237.3360 (13.34)  
test_chirp_cpu[lin-10-1-6-16384-complex]      371.0492 (20.85)  
test_chirp_cpu[log-10-1-6-16384-complex]      386.9165 (21.74)  
test_chirp_cpu[log-10-1-6-16384-real]         397.7282 (22.35)  
test_chirp_cpu[quad-10-1-6-16384-complex]     404.0797 (22.70)  
test_chirp_cpu[quad-10-1-6-16384-real]        406.1122 (22.82)  
----------------------------------------------------------------

@mnicely mnicely requested a review from awthomp January 20, 2022 16:18
@mnicely mnicely requested a review from a team as a code owner January 20, 2022 16:18
@mnicely mnicely self-assigned this Jan 20, 2022
@mnicely mnicely changed the title [REVIEW] adding complex parameter to chirp and additional tests [FEA] adding complex parameter to chirp and additional tests Jan 20, 2022
@mnicely mnicely changed the title [FEA] adding complex parameter to chirp and additional tests adding complex parameter to chirp and additional tests Jan 20, 2022
@mnicely mnicely added 3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change labels Jan 20, 2022
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.

A few comments here:

  • I'm going to change the API from dtype to type. IMO, dtype tends to define input rather than output
    * Complex output is currently only valid for a linear chirp, so I'll deprecate added functionality on quadratic and logarithmic
  • Algorithm for determining imaginary component of complex linear chirp output is incorrect.

I'll modify this PR. The framework is super handy :).

EDIT: Complex output type is supported by all chirp methods.

@awthomp
Copy link
Member

awthomp commented Jan 21, 2022

Test/Implementation script:

import numpy as np
from numpy import pi, cos

def _chirp_phase(t, f0, t1, f1, method='linear', vertex_zero=True, isComplex=False):
    t = np.asarray(t)
    f0 = float(f0)
    t1 = float(t1)
    f1 = float(f1)

    if method in ['linear', 'lin', 'li']:
        beta = (f1 - f0) / t1
        phase = 2 * pi * (f0 * t + 0.5 * beta * t * t)

    else:
        raise ValueError('method must be linear')

    return phase

def chirp(t, f0, t1, f1, method='linear', phi=0, vertex_zero=True, type='real'):
    phase = _chirp_phase(t, f0, t1, f1, method, vertex_zero, type)
    if isComplex:
      phi *= pi / 180
      return cos(phase + phi) - 1j*cos(phase + phi + pi/2)
    else:
      phi *= pi / 180
      return cos(phase + phi)

Usage

t = np.linspace(0, 10, 1500)
w = chirp(t, f0=6, f1=1, t1=10, phi=45, method='linear', isComplex=True)

For this PR, let's enable complex support for linear mode only.

@mnicely mnicely requested a review from awthomp January 22, 2022 03:32
python/cusignal/waveforms/waveforms.py Outdated Show resolved Hide resolved
@awthomp
Copy link
Member

awthomp commented Jan 23, 2022

Also, let's be sure to add type the the docs. I'm thinking:

type : {'real', 'complex'}, optional
        Specify output chirp type```

@mnicely mnicely requested a review from awthomp January 24, 2022 14:26
@awthomp awthomp changed the base branch from branch-22.02 to branch-22.04 January 24, 2022 14:53
@awthomp
Copy link
Member

awthomp commented Jan 28, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c620d82 into rapidsai:branch-22.04 Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants