[src] Multichannel models

[mpariente]

Started implementing what I suggested in #420.

I'll highlight the places where I doubt in the PR.

Funny things I found along the way:

• It's hard to make a model that doesn't take the sample rate as argument

class Model(BaseModel):
    def __init__(self):
        super().__init__(sample_rate=44100)
        
    def forward(self, x):
        return x

    def get_model_args(self):
        return {}

This can be serialized, but not loaded because sample_rate is needed in the model conf to load the model.

OTOH:

class Model(BaseModel):
    def __init__(self):
        super().__init__(sample_rate=44100)

    def forward(self, x):
        return x

    def get_model_args(self):
        return {"sample_rate": self.sample_rate}

This fails at loading, because it tries to pass sample_rate as kwargs, but cannot.
Should we do something about this?

[mpariente]

I don't think this needs to be a property as the sample_rate but we could do it.

[popcornell]

why not ? some models are tied to the number of channels and actually to the array topology

[mpariente]

For sample_rate, we made it like that because the model holds reference to the sample_rate, but the filterbank as well, so we wanted the raise the warning when setting it.

But for the number of channels, for now nothing holds reference to it.
If we see it's a limitation in the future we can always write a setter/getter.

Asteroid, as Python, is for consenting adults 😉

[mpariente]

Should we add a flag to ignore that, if passed, make us ignore that and take the first channels or something?
Something that would be passed from the CLI to here (--ignore-channels-check).
I'm not sure it's useful.

[jonashaag]

Not sure...

[mpariente]

@popcornell what's your opinion on that?
Maybe we can start by not having it. And if we find it useful later, or there is a user demand, we can change that.

[mpariente]

As said in the PR. We must have the sample_rate as argument.

If we want fixed number of channels, we don't need it in the __init__, we just super(n_channels=2) and that works.

[mpariente]

Another note: there will be several scenarios for LambdaOverlapAdd:

• Multichannel input, single output (should work fine)
• Multichannel input, multichannel output (not sure it works).

We should add some tests and if the second case doesn't work, raise a useful error.

[jonashaag]

This can be serialized, but not loaded because sample_rate is needed in the model conf to load the model.

How about moving the check for missing sample rate to after the model object has been constructed, and then checking using hasattr(model, "sample_rate")? That way you are free to set the sample_rate property however you like as long as it's present.

[mpariente]

This can be serialized, but not loaded because sample_rate is needed in the model conf to load the model.

How about moving the check for missing sample rate to after the model object has been constructed, and then checking using hasattr(model, "sample_rate")? That way you are free to set the sample_rate property however you like as long as it's present.

I thought about that.
That seems fine to me, let's do that.

[mpariente]

How about moving the check for missing sample rate to after the model object has been constructed, and then checking using hasattr(model, "sample_rate")? That way you are free to set the sample_rate property however you like as long as it's present.

sample_rate has a default value, so actually this won't work because the sample_rate property will be there.

[jonashaag]

How about we drop it? It's backwards incompatible, but loading models will still work, and it's easy to fix for people. Maybe it's time we don't default to 8 kHz anymore now that people are using Asteroid for other things than traditional 8 kHz speech separation

[mpariente]

Well, I'd be ok with that!

[mpariente]

This is enough for this PR.
After merging this, I'll create a PR to remove the default on the sample rate.