transforms.AmplitudeToDB does not handle cut-off correctly for multi-channel or batched data

[Stehsegler]

🐛 Bug

From my understanding (based on e.g. #337), all transforms should be able to operate on tensors with dimensions (batch, channels, ...) or (channels, ...), with ... being dependent on the type of data being processed, e.g.time for waveform data and freq, time for spectrograms. In this way, we can pass multiple chunks of data (waveforms, spectrograms…) at once and expect to get the same results as if we would pass them one by one.

However, this is not the case transforms.AmplitudeToDB: As easily traceable in the source code of the corresponding functional, this transform blindly operates on the passed tensor without taking its dimensionality and the related semantics into account in any way.

This becomes a problem in the calculation of the cut-off. The purpose of this step is to clamp low dB values to a minimum value some fixed amount of decibels below the maximum value in the respective spectrogram. However, amplitude_to_db uses the single global maximum in the passed tensor to calculate the cut-off for all contained spectrograms. Thus, when passing batched data, the results for one spectrogram are dependent on all the other spectrograms in the same batch, which to my understanding is not the correct behavior. My conclusion is that AmplitudeToDB silently outputs wrong (in the sense of transforms general interface contract) data for batched or multi-channel data, which I would consider really dangerous for applications.

Ideally, this should be fixed directly in functional.amplitude_to_DB, so that we can also pass batched data there.

Environment

TorchAudio 0.7.0

[vincentqb]

Thanks for opening the issue :) Are you referring to the use of x_db.max() on this line when top_db is not None? Do you have an example that would give an incorrect result?

[Stehsegler]

Thanks for opening the issue :) Are you referring to the use of x_db.max() on this line when top_db is not None?

Exactly.

Do you have an example that would give an incorrect result?

Sure. Using a batch of two 2*2 example "spectrograms":

In: spec_batch = torch.tensor([[[2.0, 1.0], [4.0, 0.0]], [[10000000000.0, 5000000000.0], [8000000000.0, 9000000000.0]]])

In: F.amplitude_to_DB(spec_batch[0], 10.0, 0.0, 1.0, 80.0)  # Processing the first spectrogram on its own yields expected result.
Out: 
tensor([[ -6.9897, -10.0000],
        [ -3.9794, -83.9794]])

In: F.amplitude_to_DB(spec_batch, 10.0, 0.0, 1.0, 80.0)  # Processing the whole batch at once yields different (incorrect) results for the first spectrogram.
Out: 
tensor([[[10.0000, 10.0000],
         [10.0000, 10.0000]],
        [[90.0000, 86.9897],
         [89.0309, 89.5424]]])

The key point is that results for individual items should be the same regardless whether they are processed individually or in a batch.

[vincentqb]

That's a very good catch, thanks! This should be replaced by a min/max, e.g. here. Do you want to open a pull request for this?

[Stehsegler]

Thanks for your feedback!

As I'm not familiar with the Torch (Audio) Codebase and especially test infrastructure, I'm afraid I won't be able to open a pull request myself.

[jcaw]

I've got a local patch for this, didn't realize there was already a ticket open. Would you like me to open a PR?

[jcaw]

I think in order to upstream this I think I need to clarify some things:

1. My understanding is that the objective here is to retain the clamping behaviour, but apply a separate clamp per-spectrogram. Is this correct?
2. What input shape should amplitude_to_DB expect? Is it always a spectrogram with N channels, requiring batches to be packed? Should it be capable of receiving an unpacked batch? Are there always two trailing dimensions, freq & time, or might the data be in a different format?
3. Should the clamping be applied per-spectrogram, or per-channel? If it's the former, that makes things more complicated as the different MFCCs are packed - so there's a single combined dimension for the batch & channel.

My patch currently clamps per-channel (not per spectrogram), and assumes a packed batch of two-dimensional spectrogram channels (so three dimensions, [channels, freq, time]).

[vincentqb]

I've got a local patch for this, didn't realize there was already a ticket open. Would you like me to open a PR?

That would great, thanks! :)

1. My understanding is that the objective here is to retain the clamping behaviour

Yes, we should retain the clamping behavior.

but apply a separate clamp per-spectrogram. Is this correct?

That's an interesting point: per channel or spectrogram? Reading at the code, I'd say the intended behavior is likely to have been per spectrogram, since the batching came later. So let's keep this behavior.

2. What input shape should amplitude_to_DB expect? Is it always a spectrogram with N channels, requiring batches to be packed? Should it be capable of receiving an unpacked batch?

Batches and channels were packed together to add batching support, so this does need to be fixed. In particular, this means the transform should likely not fold batches into channels to apply the transform.

Are there always two trailing dimensions, freq & time, or might the data be in a different format?

The format should be (..., freq, time). The function does not apply to complex tensor (which would have had a shape (..., freq, time, 2).

3. Should the clamping be applied per-spectrogram, or per-channel? If it's the former, that makes things more complicated as the different MFCCs are packed - so there's a single combined dimension for the batch & channel.

Yes, indeed, though comment above would help take care of replacing clamp by min/max.

[jcaw]

Awesome!

Reading at the code, I'd say the intended behavior is likely to have been per spectrogram, since the batching came later. So let's keep this behavior.

This also matches librosa's behavior.

[jcaw]

Working through this now - I need to clarify the intended structure.

The current test expects amplitude_to_DB to work on both waves and spectrograms. This means we can't infer the indexes of the input tensor unless we either assume the data is always batched, or always a spectrogram. Would you like two methods, amplitude_to_DB and amplitude_to_DB_batched? Or maybe amplitude_to_DB_wave and amplitude_to_DB_spec? Alternatively, it could always expect batched data or wave support could be dropped.

Edit: I'm assuming flags aren't the correct way to do this in PyTorch, e.g. mask_along_axis_iid

(The same applies to the AmplitudeToDB transform.)

[jcaw]

@vincentqb any thoughts? I have the PR ready to go, I just need to know the structure. I'm thinking following the lead of mask_along_axis and having two methods is sensible.

(Side note, but what does iid stand for specifically?)

[mthrok]

HI @jcaw

You can go ahead and open a PR. That way it is easier to keep the discussion going.
I would like to update our test infrastructure to catch this kind of bugs, so it will help to think of a way to fix the tests.

[jcaw]

Great, will do.