Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-817] Fixes to speech recognition example #12291

Merged
merged 1 commit into from
Aug 30, 2018
Merged

[MXNET-817] Fixes to speech recognition example #12291

merged 1 commit into from
Aug 30, 2018

Conversation

vandanavk
Copy link
Contributor

@vandanavk vandanavk commented Aug 22, 2018 •
edited
Loading

Description

Changing the example to use mxboard instead of tensorboard (tensorboard SummaryWriter is deprecated - as identified by #12024 (comment)). Fixing a couple of other bugs in the example.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Change tensorboard to mxboard. Edited the README and code.
  • Added a note about batch_size in README
  • Fixes some lint errors
  • Fix for Divide by Zero error in stt_metric

Comments

@vandanavk vandanavk requested a review from szha as a code owner August 22, 2018 17:36
@vandanavk vandanavk changed the title Fixes to speech recognition example [MXNET-817] Fixes to speech recognition example Aug 22, 2018
@vandanavk vandanavk force-pushed the speech branch 2 times, most recently from 50b01af to 77f22a4 Compare August 22, 2018 18:15
@vandanavk vandanavk mentioned this pull request Aug 22, 2018
Closed
@vandanavk
Copy link
Contributor Author

vandanavk commented Aug 22, 2018 •
edited
Loading

@sbodenstein @Soonhwan-Kwon

anirudhacharya
anirudhacharya reviewed Aug 22, 2018
View reviewed changes
Copy link
Member

@anirudhacharya anirudhacharya left a comment

Choose a reason for hiding this comment

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

has the whole example been tested after switching from tensorboard to mxboard?

example/speech_recognition/singleton.py Outdated
@@ -32,7 +32,7 @@ def getInstance(self):

def __new__(class_, *args, **kwargs):
print("__new__")
class_.instances[class_] = super(Singleton, class_).__new__(class_, *args, **kwargs)
class_.instances[class_] = super().__new__(class_, *args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

why remove passing Singleton and class_ while calling super?

Typo in line 42, should be SingletonInstance.
line 52 in redundant.
The whole SingletonInstance class defined in line 42 is redundant. It is not being used anywhere. I think it can be removed.

In fact in this whole speech recognition example, the code around the use of singletons can be refactored. The Singleton class defined in line 21 is used only once as a decorator in label_util.py module.

The log_util.py module again defines another class called SingletonType which is used to make LogUtil class a singleton. There is duplication of code and logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python3 complained about this usage of super(Singleton, class_).
LogUtil's singleton looks much cleaner. Will refactor the code around this. Wanted to make minimal changes to get the example working/not break it - but I think this is essential. Will make the changes and resubmit. Thanks @anirudhacharya

example/speech_recognition/stt_metric.py Outdated
@@ -86,7 +86,10 @@ def update(self, labels, preds):
def get_batch_loss(self):
return self.batch_loss
def get_name_value(self):
total_cer = float(self.total_l_dist) / float(self.total_n_label)
try:
Copy link
Member

Choose a reason for hiding this comment

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

nit: newlines between methods is inconsistent in this file.

@sandeep-krishnamurthy sandeep-krishnamurthy added Python Example pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Aug 23, 2018
@vandanavk
Copy link
Contributor Author

Testing has been done for a small dataset. Training for the entire LibriSpeech dataset has been going on for 2 days now.

@sandeep-krishnamurthy sandeep-krishnamurthy added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Aug 24, 2018
sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Aug 24, 2018
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for your contributions @vandanavk

@vandanavk
Copy link
Contributor Author

Is this PR good to go?

@anirudhacharya
Copy link
Member

LGTM for most parts. Is the singleton refactor being tracked anywhere?

@vandanavk vandanavk mentioned this pull request Aug 28, 2018
Closed
@vandanavk
Copy link
Contributor Author

vandanavk commented Aug 28, 2018 •
edited
Loading

@anirudhacharya Opened an issue #12384 for tracking the refactor

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit e456dc4 into apache:master Aug 30, 2018
aaronmarkham pushed a commit to aaronmarkham/incubator-mxnet that referenced this pull request Sep 11, 2018
@vandanavk @aaronmarkham
Fix speech recognition example (apache#12291)
970b3cd
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Sep 19, 2018
@vandanavk @anirudh2290
Fix speech recognition example (apache#12291)
77e173f
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@anirudhacharya anirudhacharya anirudhacharya left review comments

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
Example pr-awaiting-review PR is waiting for code review Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vandanavk @anirudhacharya @sandeep-krishnamurthy