Hilbert UGens - phase convention requires more detailed documentation OR update

[joslloand]

Introduction

There are two different phase (rotation) conventions that have been adopted in the DSP community for specification of the Hilbert transform.

1. [ cos', sin' ]
2. [ cos', -sin' ]

Given an input, two outputs are expected:

• a "unit" output
• a Hilbert "phase shifted" output

The "unit" output is a phase (or time lagged) all-pass filtered signal. With respect to the unit output, the Hilbert output either lags by 90 deg, Convention 1, or leads Convention 2. This second output should also be all-pass.

Illustrating in terms of phase:

1. [ phase lag X, phase lag X - 90 deg ]
2. [ phase lag X, phase lag X + 90 deg ]

Discussion

Convention 1 is the form suggested by JULIUS O. SMITH III in HILBERT TRANSFORM DESIGN EXAMPLE. Zolzer, et al., also advocate this convention in DAFX: Digital Audio Effects, 2nd Edition. Additionally, Wikipedia also highlights this form.

At the moment SC3's Hilbert and HilbertFIR UGens appear to have adopted the latter form. (Note: I'm one of the authors of the original Hilbert code, AND, there is an output naming discrepancy to be found the source code for Hilbert: signal var labels for cos and sin are swapped, in error. Either My Bad, or Sean Costello Bad; Costello and I have briefly discussed this.)

Aside from consistency with reference sources, there is an advantage in adopting Convention 1 over Convention 2, in that an Analytic Signal can easily be constructed by mapping the two real outputs to the complex domain:

[ cos', sin' ] --> [ real, imag ]

Solutions

The most simple solution would be just to document the current behavior of the two Hilbert UGens. And provide some example code illustrating. Also, adding a discussion to PV_PhaseShift90 would also be useful. Here's a gist with some examps.

My preferred solution would be to both document and adopt Convention 1. The FreqShift UGen would also need to be updated, too.

[nhthn]

if I have the math right, the only difference between conventions 1 and 2 is a phase inversion, right? in that case this is a particularly painless API change. emulating the old behavior is accomplished by multiplying by -1.

as for whether the API change would be disruptive, I'm not sure. Hilbert and HilbertFIR don't seem to be used much. FreqShift is definitely used, but I doubt most of its musical applications would be seriously affected by inverting the phase.

if we divide SC users into composers and DSP people, the DSP people will be happier and the composers won't care. so i don't see any reason why not.

[telephon]

There was a point in sc2 where James McCartney changed the parameter scaling in Klank (a very commonly used UGen). It was announced and a new UGen Klank_old added, which you could use for backward compatibility. Maybe that could be a way to deal with this?

[telephon]

Alternatively, we can deprecate Hilbert.

[danstowell]

I'd say just go for it, make the change. For the reasons already described: unlikely many people will suffer, and if anyone does, patching their code is easy.

[vivid-synth]

Is there a place we're keeping track of these breaking changes?

[joslloand]

Responding to to @snappizz

if I have the math right, the only difference between conventions 1 and 2 is a phase inversion, right? in that case this is a particularly painless API change. emulating the old behavior is accomplished by multiplying by -1.

In an abstract sense, true. In looking closer, the actual implementations of Hilbert and HilbertFIR have two separate issues, however.

The Hilbert UGen is actually a Phase Difference Network (PDN). As a result, the "unit" response should be the output with the lowest group delay. As a result, the fix for Hilbert would be to swap the two outputs.

The HilbertFIR UGen is a Phase Vocoder resynthesis, where the phase of each window has been rotated by +90deg. The (short term) fix for HilbertFIR could just be to multiply the 2nd output by -1, as noted by @snappizz . Another (short term) fix could be to substitute in a -90deg rotation using PV_PhaseShift270, instead. This 2nd approach matches the actual math, better.

[joslloand]

Re @danstowell comment, and particularly @snappizz

DSP people will be happier

Sounds like we're 👍 for going ahead with some changes to Hilbert implementations.

Related to @telephon

Alternatively, we can deprecate Hilbert.

I am wondering about renaming to make things clearer?:

• Hilbert --> HilbertPDN
• HilbertFIR --> HilbertPV

An algorithm change was made some time ago to HilbertFIR. The algorithm was changed from using convolution with a Hilbert FIR kernel to implementing PV rotation. As a result, the UGen name no longer actually indicates the method being used. Also, PV rotation does introduce windowing artefacts, which are not always desirable. (I'm planning to look more closely at these, actually.)

[nhthn]

Deprecating and introducing new names is a good choice, since it avoids an API change. To be fair to other clients, let's avoid making any of the four names pseudo-UGens in the process. That is, we don't want to be lazy and do something like this:

Hilbert {
    *ar { |in, mul = 1, add = 0|
        this.deprecated(thisMethod, HilbertPDN.class.findMethod(\ar));
        ^HilbertPDN.ar(in, mul, add) * [1, -1];
    }
}

[nhthn]

@vivid-synth here's the changelog