FasNet TAC integration

[popcornell]

As the title implies. It was about time.

Doubts I have:

1. where to put the model and its components (especially the NCC module probably is better to move it in dsp)
   (IMO in future we will have to refactor and re-organize the whole masknn ).
2. the .pkl for the dataset generation should be moved elsewhere probably or fetched directly by cloning luo's repo.
3. eval script missing right now (TODO)
4. also docstrings (TODO)

[mpariente]

Cool, thanks !

IMO in future we will have to refactor and re-organize the whole masknn

Can you explain why, in your opinion, please?

5. Tests missing (TODO)

[mpariente]

Another thing: I know that if we merge this without squashing, the binary files will be in the git history, even if removed from the PR. This would be problematic because they are quite heavy.

But, I'm not sure what happens if we squash. My intuition is that they won't be in history. I know @iver56 has some knowledge about this, do you know about this particular case?

[iver56]

Another thing: I know that if we merge this without squashing, the binary files will be in the git history, even if removed from the PR. This would be problematic because they are quite heavy.

But, I'm not sure what happens if we squash. My intuition is that they won't be in history. I know @iver56 has some knowledge about this, do you know about this particular case?

I'm not an expert on this topic, but I think if you squash the commits (where one of them added a file and a later one removed it again), the squashed commit does not include the file. So I think if you include only the squashed commit, and discard the original, offending commits, the repository won't contain the big file. Maybe git gc is needed for pruning loose objects. You can check the repository size at https://api.github.com/repos/mpariente/asteroid

[mpariente]

Thanks a lot for the feedback and resources Iver !

[popcornell]

Your call really IMO if we squash there should not be in the history as long as I remove them. If we wanna be sure i can make another pull request.

Sure, IMO we have to be more modular and distinguish between models and building blocks.
For example: in attention.py there is DPTNet and is imported and wrapped in models.
I think in attention.py only building blocks should be included. And i would rename masknn to nnet or blocks or smthing like that.
I would put both DPTNet and DPTAsNet in models.

[mpariente]

Your call really IMO if we squash there should not be in the history as long as I remove them. If we wanna be sure i can make another pull request.

Let's keep it that way. I think it'll be ok.

You say that mask nets should be considered as models, it makes sense.
I don't have a clear idea of how to organize it better, or why it is currently limiting. I'm happy to discuss about that.

[JorisCos]

Thanks for the PR ! Looks great ! I will take a look again once everything is added :)

[popcornell]

What do you think about the NCC module we move it to DSP ?

[JorisCos]

What do you think about the NCC module we move it to DSP ?

IMO yes

[popcornell]

Also keep in mind this is the first step towards Multi-Channel support so we may want also to start to think about how to organize the toolkit also to support multi-channel separation and enhancement algorithms and building blocks.

[mpariente]

@popcornell do you feel like writing simple docstrings on the parts in the source code? Reviewing will be easier from there.

[popcornell]

Yes but i have limited time available this week

[mpariente]

We can almost merge. Things to sort out before:

• Location of TACDataset
• Clarification on the config etc..
• Rebase for the multichannel support, and tests serialization.
• Test the broadcasting of xcorr.

[mpariente]

\rebase

[mpariente]

/rebase

[mpariente]

Also, the model currently only supports 3D inputs, which won't work with inference.
I don't know what to do about that.
In BaseEncoderMaskerDecoder, we have these three funcs to handle input shapes in a jitable way, should we also integrate in FasNetTAC?
IMO, we can merge like this now and see if somebody can work on the jitability afterwards, maybe then, move these shape related stuff into BaseModel which can be useful in other models.

[mpariente]

I think there is a confusion in the enc_dim and feature_dim because if I change one without the other, the forward doesn't work.

[mpariente]

Great, the problem seems to be solved for feature_dim, there is still a problem with enc_dim though..

[popcornell]

Now should be solved. I extended the test also

[mpariente]

GREAT !