Angular loss1.0 #1101

nithinraok · 2020-09-01T19:49:34Z

Added angular loss with cosine angle for 1.0
Fixed multigpu metric issue by reusing classficationtopkaccuracy
Added support for embedding extraction for speaker diarization

nemo/utils/exp_manager.py

blisc

Mostly LGTM

nemo/collections/asr/losses/angularloss.py

lgtm-com · 2020-09-02T21:01:06Z

This pull request fixes 2 alerts when merging c6529f6 into 2ab5b64 - view on LGTM.com

fixed alerts:

2 for Unused import

titu1994

Some minor changes to pertinent to the model itself, and some major concerns regarding logging callbacks.

examples/speaker_recognition/spkr_get_emb.py

titu1994 · 2020-09-03T20:11:16Z

nemo/collections/asr/data/audio_to_label.py

+        slice_length = self.featurizer.sample_rate * self.time_length
+        _, audio_lengths, _, tokens_lengths = zip(*batch)
+        slice_length = min(slice_length, max(audio_lengths))
+        shift = 1 * 16000


Hardcoded sample_rate? Replace with featurizer.sample_rate

Thanks missed it, Done

titu1994 · 2020-09-03T20:13:54Z

nemo/collections/asr/losses/angularloss.py

+        """
+        return {"loss": NeuralType(elements_type=LossType())}
+
+    def __init__(self, s=20.0, m=1.35):


No option to override epsilon for other tasks? Add default eps=1e-7.

Also, dont use 1 character names for variables. And add docstring to this class.

eps is not a parameter, its to avoid negligible division by zero. Yes, I'll add docstring. s and m are very well known short forms in angular loss literature for scale and margin. If it is compulsory I will look.

titu1994 · 2020-09-03T20:15:03Z

nemo/collections/asr/losses/angularloss.py

+        super().__init__()
+
+        self.eps = 1e-7
+        self.s = s


Again, dont save variables with 1 char names. If its from a paper, add a reference section and explain what this variable is supposed to do (better yet, just use a descriptive name).

titu1994 · 2020-09-03T20:16:54Z

nemo/collections/asr/models/label_models.py

-        self.loss = CELoss()
+        if 'angular' in cfg.decoder.params and cfg.decoder.params['angular']:
+            logging.info("Training with Angular Softmax Loss")
+            s = cfg.loss.s


Config needs to have descriptive names, not one char variable name.

titu1994 · 2020-09-03T20:21:39Z

nemo/collections/asr/modules/conv_asr.py

+        self,
+        feat_in,
+        num_classes,
+        emb_sizes=[1024, 1024],


Dont directly use lists here, use None and check for None below and create [1024, 1024] if None. Refer https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

titu1994 · 2020-09-03T20:23:57Z

nemo/collections/asr/modules/conv_asr.py

            embs.append(emb)

+        if self.angular:
+            for W in self.final.parameters():
+                _ = F.normalize(W, p=2, dim=1)


https://pytorch.org/docs/master/nn.functional.html#torch.nn.functional.normalize

F.normalize is not an inplace op unless you use out=, so whats the point of this loop then?

It just normalizes the weights before calculating loss. I missed W = here, thanks Som

titu1994 · 2020-09-03T20:26:47Z

nemo/collections/common/callbacks/callbacks.py

+                    batch_idx + 1,
+                    total_batches,
+                    pl_module.loss_value,
+                    pl_module.accuracy,


Not all modules have accuracy so this callback will fail for a lot of models. Why not just read the log in its entirety and just print all of the values in the log? Cant we access the log at the end of train_batch_end?

Unfortunately not yet, PTL is working on it

titu1994 · 2020-09-03T20:29:25Z

nemo/collections/common/callbacks/callbacks.py

+
+    @rank_zero_only
+    def on_train_batch_end(self, trainer, pl_module, batch, batch_idx, dataloader_idx):
+        print_freq = trainer.row_log_interval


This will print every single batch (since PTL default is 10, nemo default is 1, not 1.0).

yes, its provided by user based on what % of num_batches or exact number he/she requires.

titu1994 · 2020-09-03T20:30:35Z

nemo/collections/common/callbacks/callbacks.py

+            )
+
+    def on_validation_epoch_end(self, trainer, pl_module):
+        logging.info(


Same, not all models have accuracy, so this will crash for them. is there a way to access the log dictionary itself?

Signed-off-by: nithinraok <[email protected]>

lgtm-com · 2020-09-03T23:29:18Z

This pull request introduces 2 alerts and fixes 3 when merging fdd898d into 292e2fb - view on LGTM.com

new alerts:

1 for Testing equality to None
1 for Variable defined multiple times

fixed alerts:

3 for Unused import

titu1994

Minor comments

titu1994 · 2020-09-04T00:42:40Z

examples/speaker_recognition/spkr_get_emb.py

@@ -49,12 +39,15 @@
 def main(cfg):

    logging.info(f'Hydra config: {cfg.pretty()}')
-    trainer = pl.Trainer(logger=False, checkpoint_callback=False)
+    if cfg.trainer.gpus > 1:


Wait do this only during inference (trainer.test()) otherwise you can't use multi GPU training

spkr_get_emb.py is only run for inference purposes.

examples/speaker_recognition/spkr_get_emb.py

nemo/collections/asr/losses/angularloss.py

nemo/collections/asr/modules/conv_asr.py

titu1994

Looks good to go. I'll let @fayejf look it over for comments and then let's merge

titu1994 · 2020-09-04T04:07:54Z

examples/speaker_recognition/spkr_get_emb.py

@@ -49,12 +39,15 @@
 def main(cfg):

    logging.info(f'Hydra config: {cfg.pretty()}')
-    trainer = pl.Trainer(logger=False, checkpoint_callback=False)
+    if cfg.trainer.gpus > 1:


lgtm-com · 2020-09-04T04:25:58Z

This pull request introduces 2 alerts and fixes 3 when merging 8e2cd41 into b5ecf8f - view on LGTM.com

new alerts:

1 for Testing equality to None
1 for Variable defined multiple times

fixed alerts:

3 for Unused import

lgtm-com · 2020-09-04T06:10:25Z

This pull request introduces 2 alerts and fixes 3 when merging 8007677 into e9d98c6 - view on LGTM.com

new alerts:

1 for Testing equality to None
1 for Variable defined multiple times

fixed alerts:

3 for Unused import

fayejf

Looks good to me! Just have two minor questions.

examples/speaker_recognition/speaker_reco.py

nemo/collections/asr/data/audio_to_label.py

jainal09 · 2020-10-02T22:14:31Z

hey @nithinraok i want to perform speaker diarialisation providing a audio file and getting multi speaker transcript ( stt of identified speakers) how to do that with this pr?

nithinraok · 2020-10-02T23:23:00Z

You could extract embeddings with this script, and use those frame level embeddings to perform Spectral Clustering by mentioning num_speakers as number of clusters. We don't have this as a to go unified script for now, but all the individual pieces are already there. I will for sure add in next coming weeks with more features.

jainal09 · 2020-10-03T16:45:33Z

Thanks and waiting for these for a long time. @nithinraok

docs: missing space

nithinraok requested a review from blisc September 1, 2020 19:49

blisc reviewed Sep 1, 2020

View reviewed changes

nemo/utils/exp_manager.py Outdated Show resolved Hide resolved

nithinraok force-pushed the angularLoss1.0 branch from 48c3048 to 2c19ad3 Compare September 2, 2020 17:33

nithinraok requested a review from blisc September 2, 2020 18:06

blisc reviewed Sep 2, 2020

View reviewed changes

nemo/collections/asr/losses/angularloss.py Outdated Show resolved Hide resolved

blisc requested review from fayejf and titu1994 September 2, 2020 18:46

NVIDIA deleted a comment from lgtm-com bot Sep 2, 2020

titu1994 requested changes Sep 3, 2020

View reviewed changes

nithinraok added 12 commits September 3, 2020 16:14

angular loss for 1.0

09a0d85

Signed-off-by: nithinraok <[email protected]>

metrics update

41081d8

Signed-off-by: nithinraok <[email protected]>

metric update

b97b940

Signed-off-by: nithinraok <[email protected]>

logvallcallback

2ba342e

Signed-off-by: nithinraok <[email protected]>

CallbackManager

0bb0092

Signed-off-by: nithinraok <[email protected]>

Updated spkr_get_emb to support diarization

7d8bdf6

Signed-off-by: nithinraok <[email protected]>

callback in expmanager

965a517

Signed-off-by: nithinraok <[email protected]>

Removed callback from exp_mager will push another PR for it

f593c38

Signed-off-by: nithinraok <[email protected]>

LGTM

2a621f4

Signed-off-by: nithinraok <[email protected]>

experimenta flag

a84195e

Signed-off-by: nithinraok <[email protected]>

moved callback to new PR added doc strings

df8f8c2

Signed-off-by: nithinraok <[email protected]>

style fix

fdd898d

Signed-off-by: nithinraok <[email protected]>

nithinraok force-pushed the angularLoss1.0 branch from c6529f6 to fdd898d Compare September 3, 2020 23:20

nithinraok requested a review from titu1994 September 3, 2020 23:20

titu1994 requested changes Sep 4, 2020

View reviewed changes

titu1994 reviewed Sep 4, 2020

View reviewed changes

titu1994 approved these changes Sep 4, 2020

View reviewed changes

Merge branch 'main' into angularLoss1.0

8e2cd41

Merge branch 'main' into angularLoss1.0

8007677

fayejf approved these changes Sep 4, 2020

View reviewed changes

examples/speaker_recognition/speaker_reco.py Show resolved Hide resolved

nemo/collections/asr/data/audio_to_label.py Show resolved Hide resolved

nithinraok merged commit c765631 into main Sep 4, 2020

nithinraok deleted the angularLoss1.0 branch September 4, 2020 18:36

dcurran90 pushed a commit to dcurran90/NeMo that referenced this pull request Oct 15, 2024

Merge pull request NVIDIA#1101 from booxter/space-missing

aa71069

docs: missing space

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Angular loss1.0 #1101

Angular loss1.0 #1101

nithinraok commented Sep 1, 2020 •

edited

Loading

blisc left a comment

lgtm-com bot commented Sep 2, 2020

titu1994 left a comment

titu1994 Sep 3, 2020

nithinraok Sep 3, 2020

titu1994 Sep 3, 2020

nithinraok Sep 3, 2020 •

edited

Loading

titu1994 Sep 3, 2020

titu1994 Sep 3, 2020

titu1994 Sep 3, 2020

nithinraok Sep 3, 2020

titu1994 Sep 3, 2020

nithinraok Sep 3, 2020

titu1994 Sep 3, 2020

nithinraok Sep 3, 2020

titu1994 Sep 3, 2020

nithinraok Sep 3, 2020

titu1994 Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

titu1994 left a comment

titu1994 Sep 4, 2020

nithinraok Sep 4, 2020

titu1994 Sep 4, 2020

titu1994 left a comment

titu1994 Sep 4, 2020

lgtm-com bot commented Sep 4, 2020

lgtm-com bot commented Sep 4, 2020

fayejf left a comment

jainal09 commented Oct 2, 2020

nithinraok commented Oct 2, 2020 •

edited

Loading

jainal09 commented Oct 3, 2020

Angular loss1.0 #1101

Angular loss1.0 #1101

Conversation

nithinraok commented Sep 1, 2020 • edited Loading

blisc left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 2, 2020

titu1994 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithinraok Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 3, 2020

titu1994 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titu1994 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 4, 2020

lgtm-com bot commented Sep 4, 2020

fayejf left a comment

Choose a reason for hiding this comment

jainal09 commented Oct 2, 2020

nithinraok commented Oct 2, 2020 • edited Loading

jainal09 commented Oct 3, 2020

nithinraok commented Sep 1, 2020 •

edited

Loading

nithinraok Sep 3, 2020 •

edited

Loading

nithinraok commented Oct 2, 2020 •

edited

Loading