Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix time stamps #2522

Merged
merged 9 commits into from
Jul 21, 2021
Merged

Fix time stamps #2522

merged 9 commits into from
Jul 21, 2021

Conversation

nithinraok
Copy link
Collaborator

@nithinraok nithinraok commented Jul 21, 2021 •
edited
Loading

  • Update timestamps to calculate only once
  • introduce segmentation module
  • support multi-batch inference for speaker embedding extraction

@lgtm-com
Copy link

lgtm-com bot commented Jul 21, 2021

This pull request introduces 2 alerts when merging 1e79725 into c8f9427 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@nithinraok nithinraok marked this pull request as ready for review July 21, 2021 01:23
@okuchaiev
Copy link
Member

is this a bugfix? should this go to r1.2.0?

@nithinraok
Copy link
Collaborator Author

nithinraok commented Jul 21, 2021 •
edited
Loading

not a bug fix, but an improvement over what we had

tango4j
tango4j reviewed Jul 21, 2021
View reviewed changes
Copy link
Collaborator

@tango4j tango4j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all, there seems to be no major issue but it would be better off we use consistent variable name over the whole codebase.

sub_segment, sub segment ---> subsegment

for both doc string and variables.

nemo/collections/asr/parts/utils/speaker_utils.py Outdated Show resolved Hide resolved
nemo/collections/asr/parts/utils/speaker_utils.py Outdated Show resolved Hide resolved
nemo/collections/asr/parts/utils/speaker_utils.py Outdated Show resolved Hide resolved
nemo/collections/asr/parts/utils/speaker_utils.py Outdated Show resolved Hide resolved
nemo/collections/asr/parts/utils/speaker_utils.py Outdated Show resolved Hide resolved
titu1994
titu1994 reviewed Jul 21, 2021
View reviewed changes
Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks fine, perform the requested changes and then @tango4j for final review

examples/speaker_recognition/conf/speaker_diarization.yaml Show resolved Hide resolved
@nithinraok
time stamps done
e5c5650 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
multi batch_size support
9271684 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
add doc strings
9555744 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
spelling fix
08f0bdc 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
bs default
c9a0153 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
test jenkins remove file
633b39f 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
revert file not found jenkins fix
8b04b80 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
subsegments rename
e782e17 
Signed-off-by: nithinraok <[email protected]>
@nithinraok
out_dir check
5d3f8ff 
Signed-off-by: nithinraok <[email protected]>
tango4j
tango4j approved these changes Jul 21, 2021
View reviewed changes
Copy link
Collaborator

@tango4j tango4j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the required changes are all reflected.

@nithinraok nithinraok merged commit 9fad1dc into main Jul 21, 2021
@nithinraok nithinraok deleted the fix_time_stamps branch July 21, 2021 21:42
blisc pushed a commit to blisc/NeMo that referenced this pull request Aug 12, 2021
@nithinraok @blisc
Fix time stamps (NVIDIA#2522)
696d2ea 
* time stamps done

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

* multi batch_size support

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

* add doc strings

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

* spelling fix

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

* bs default

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

* test jenkins remove file

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

* revert file not found jenkins fix

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

* subsegments rename

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

* out_dir check

Signed-off-by: nithinraok <[email protected]>
Signed-off-by: Jason <[email protected]>
paarthneekhara pushed a commit to paarthneekhara/NeMo that referenced this pull request Sep 17, 2021
@nithinraok @paarthneekhara
Fix time stamps (NVIDIA#2522)
5c0abe6 
* time stamps done

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

* multi batch_size support

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

* add doc strings

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

* spelling fix

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

* bs default

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

* test jenkins remove file

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

* revert file not found jenkins fix

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

* subsegments rename

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

* out_dir check

Signed-off-by: nithinraok <[email protected]>
Signed-off-by: Paarth Neekhara <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@titu1994 titu1994 titu1994 left review comments

@tango4j tango4j tango4j approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nithinraok @okuchaiev @titu1994 @tango4j