Speech Regression Model
State: closed | Pull request: yes

Extend the current speech classification models (e.g. Speaker Recognition) to accommodate the prediction of a numeric target (regression). 
This can be applied to use cases like [Speech Quality Assessment ](https://arxiv.org/pdf/1903.06908.pdf)

This pull request **introduces 1 alert** when merging 657e3e50296a8181164b830a812b40858d5917a6 into bc5f03459c7273b71c50af7c2378d5e950b408c4 - [view on LGTM.com](https://lgtm.com/projects/g/NVIDIA/NeMo/rev/pr-a3b2ef6ca144959b67e47fc21de10d8f7c6251b2)

**new alerts:**

* 1 for &#39;import \*&#39; may pollute namespace

Hi @titu1994, I would like to merge this pull request. Could you review it? Thanks in advance

This pull request **introduces 1 alert** when merging b4cbe146d2d376a805cdb183615068a06d38683a into bc5f03459c7273b71c50af7c2378d5e950b408c4 - [view on LGTM.com](https://lgtm.com/projects/g/NVIDIA/NeMo/rev/pr-8ac0d3b412e9f286a1c6b459f6d1ec28aaf35deb)

**new alerts:**

* 1 for &#39;import \*&#39; may pollute namespace

I completely agree with your comments, there are clear opportunities to avoid code duplication. I've implemented this way for the sake of clarity and to avoid conflicts. Anyway, I will watch the related PR and refactor this one after it's been merged. 

On the other hand, the DCO check is failing as my commits have been signed with a wrong email. I've tried to correct it by rebasing, but I wasn't able to fix it so far. Any clue about how to handle it?

Thanks for your help

HI @diego-fustes Sorry for keep you waiting. [PR#1617](https://github.com/NVIDIA/NeMo/pull/1617) is merged now 😃 . Please rebase on it. And let me know if you have any questions.

This pull request **introduces 1 alert** when merging 33d6fd0525ba7728f9d34aaa19fa45f29d038c47 into 14904fd0381f5515a9773cf7a538dc09e08c8aa9 - [view on LGTM.com](https://lgtm.com/projects/g/NVIDIA/NeMo/rev/pr-ff942955051d87be1008417b4aab787ce24f8579)

**new alerts:**

* 1 for &#39;import \*&#39; may pollute namespace

Hi @fayejf and @titu1994 :

I've been looking at the current code base, and I think that some refactoring is still needed. To start with, I think that EndDecSpeakerLabelModel should be a subclass of EncDecClassificationModel. At the end, I think that it's almost the same, just with the option of switching the loss to AngularSoftmaxLoss and that it outputs the embeddings along with the logits. 

On the other hand, the class EncDecRegressionModel that I'm implementing should be a superclass of EncDecClassificationModel. At the end, classification can be seem as a special type of regression with a threshold...

What do you think? It's quite  a major change, I'm willing to implement it if you agree 

Hey @diego-fustes,

You raise some are valid points, but there are other reasons they are kept separate as of now.

> EndDecSpeakerLabelModel should be a subclass of EncDecClassificationModel. At the end, I think that it's almost the same, just with the option of switching the loss to AngularSoftmaxLoss and that it outputs the embeddings along with the logits.

Yes, this is true - for the present. The reason we internally decided to separate the two models is that Speech classification is guarenteed to have a 1 ip - 1 op pipeline as in general classification models. Speaker classification on the other hand may use auxiliary information in the future, or might require architectures with multiple outputs. Speaker classification is also one step in the entire speaker diarization pipeline, which needs different interaction than the simpler classification model. All in all, it was therefore concluded that albeit they share significant similarities, they would still be separate entities.

> EncDecRegressionModel that I'm implementing should be a superclass of EncDecClassificationModel. At the end, classification can be seem as a special type of regression with a threshold

This is something I disagree with. Simply put, while mathematically correct that classification is a special case of regression, that does not mean it is semantically the same task. We dont apply regression losses (MSE) to train MNIST / ImageNet, nor do we use any activation function for regression models (as an example). The dataset, and the semantic task that the model performs is entirely different, even if mathematically the only difference is the application of softmax/sigmoid at the end of the model.


This pull request **introduces 1 alert** when merging 01e79acda2faf0f9b38ef58d73bc1564a10cb4ae into 941ef1fd71bd2515a4ba7092d65146edfddc1229 - [view on LGTM.com](https://lgtm.com/projects/g/NVIDIA/NeMo/rev/pr-460d53983a0a2e4ee69ca4a17081e35c2bc9dda9)

**new alerts:**

* 1 for &#39;import \*&#39; may pollute namespace

Hi @titu1994 

I agree with you, from a practical perspective it's better to implement the common code for regression and classification in an abstract class. I've just pushed a new version, trying to minimize code duplication. Apologies as I think that I've added more commits that needed, I'm a kind of starting with GitHub.

Kind regards

This pull request **introduces 1 alert** when merging 1452075151885cb3a3395d768d4603dac6c16ef5 into 941ef1fd71bd2515a4ba7092d65146edfddc1229 - [view on LGTM.com](https://lgtm.com/projects/g/NVIDIA/NeMo/rev/pr-8cc3f74e16c23909b4cedfe02da85605b1763535)

**new alerts:**

* 1 for &#39;import \*&#39; may pollute namespace

Hey @diego-fustes, no worries about Git. Could you try rebasing your commit on top of main branch instead of merge? That should clean up the commit history. 

I was looking though some of the changes, and they are pretty good. Some minor cleanup here and there and it should be good to merge. I'll post the comments after the rebase (as comments sometime get deleted after rebase)