Made changes to the augmentation script to make it work for ASR and speaker ID

[phanisankar-nidadavolu]

No description provided.

[danpovey]

@david-ryan-snyder please LMK if this is good to merge

[danpovey]

Some smallish comments. Big question is what to do with ivectors and CMN and whether to make those changes at the same time as this.

[danpovey]

I think I'd like this script to exit with an error if it detects that what it is trying to create already exists.

[danpovey]

Since we are re-using this script I think it would be better to work on it a bit more carefully-- e.g. use argparse to parse the command line args (with something more standard than the 'Y' to use the vocals, i.e. make it a boolean flag)-- and have a proper usage message printed out if called wrongly.
Also, I think we can put this in a shared place, steps/augmentation/ or wherever, since other scripts will likely want to call it. The same goes for the shell script-- let's put it somewhere shared, e.g. in the same place; have it check whether its input already exists; give it a usage message; and have it parse command line args properly (e.g. give it a --use-vocals option). Also some kind of function-level documentation for this python script would be nice (e.g. a doc-string).

[danpovey]

put the normal apache header on this please, and your name.

[danpovey]

Also can you please add an arg use_ivectors=true that wil give the option to skip all the i-vector-related parts of the script? We may want to use this option later on when we switch to online CMN, but for now I guess we can leave that issue separate, I suppose there is no need to mix it up with this.

[danpovey]

I think you can break this up into more stages, in case it dies somewhere in the middle. And please have more of the stages check whether their work has already been done and die with a suitable message if so.

[danpovey]

I think we should remove the multi-style option here, for purposes of experiments, since if people didn't want it, they just shouldn't call this. I know this was useful for you... you can of course use it for your own experiments, but i would have though just settinng noise_list to "clean" would do what you want.
Also I don't like the name "noise_list" as reverb and clean are not noise. Let's call it "augmentation_list" wherever it occurs.

[danpovey]

An example of how to register boolean options:

    parser.add_argument("--chain.apply-deriv-weights", type=str,
                        dest='apply_deriv_weights', default=True,
                        action=common_lib.StrToBoolAction,
                        choices=["true", "false"],
                        help="")

[danpovey]

let's call this --modify-spk-id .. more consistent with spk2utt, etc.

[danpovey]

please try to mostly keep within 80 or 100 characters by breaking long lines.

[danpovey]

It's good that you are adding documentation, but let's do it with a doc-string, e.g.

def Foo(string):
     """ Foo is a function that does nothing.
               'string' is expected to be of type str, but is otherwise ignored.
          This function returns None.
     """
      return None

[jty016]

I think this script can be used for 16000 Hz sampled audio too, how about 8k as a parameter

[danpovey]

@david-ryan-snyder or @phanisankar-nidadavolu, perhaps you can comment? And @phanisankar-nidadavolu, let me know if I should be merging this now.

[phanisankar-nidadavolu]

Hello Dan, I think it is a good observation. musan corpus is originally at 16KHz. When we use this script (musan.py) for augmenting 8k data (like swbd, sre16) we downsample musan corpus to 8kHZ (like in the below line) utt2wav_str = utt2wav_str + utt + " sox -t wav " + utt2wav[utt] + " -r 8k -t wav - |\n" if the corpus is at 16k (like SITW) this is not required. Ideal setup would be to make sampling frequency optional and use same musan.py everywhere. But since musan scripts are located in local directories, it is safe enough to assume the sampling frequency is known in advance (you know whether to resample it or not). Let me know what you think. Regarding merging I will push my latest changes tonight and we can merge. Phani

…

On Thu, Apr 18, 2019 at 5:48 PM Daniel Povey ***@***.***> wrote: @david-ryan-snyder <https://github.com./david-ryan-snyder> or @phanisankar-nidadavolu <https://github.com./phanisankar-nidadavolu>, perhaps you can comment? And @phanisankar-nidadavolu <https://github.com./phanisankar-nidadavolu>, let me know if I should be merging this now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AH3QA2UWX2EEPUDWYR56Q23PRDUC7ANCNFSM4G622LCA> .

[danpovey]

It sounds like a great idea. Please, if you have time, either implement it yourself or work with the person who raised the issue to do so if you feel that is practical. On Thu, Apr 18, 2019 at 3:09 PM phanisankar-nidadavolu < [email protected]> wrote:

…

Hello Dan, I think it is a good observation. musan corpus is originally at 16KHz. When we use this script (musan.py) for augmenting 8k data (like swbd, sre16) we downsample musan corpus to 8kHZ (like in the below line) utt2wav_str = utt2wav_str + utt + " sox -t wav " + utt2wav[utt] + " -r 8k -t wav - |\n" if the corpus is at 16k (like SITW) this is not required. Ideal setup would be to make sampling frequency optional and use same musan.py everywhere. But since musan scripts are located in local directories, it is safe enough to assume the sampling frequency is known in advance (you know whether to resample it or not). Let me know what you think. Regarding merging I will push my latest changes tonight and we can merge. Phani On Thu, Apr 18, 2019 at 5:48 PM Daniel Povey ***@***.***> wrote: > @david-ryan-snyder <https://github.com./david-ryan-snyder> or > @phanisankar-nidadavolu <https://github.com./phanisankar-nidadavolu>, > perhaps you can comment? And @phanisankar-nidadavolu > <https://github.com./phanisankar-nidadavolu>, let me know if I should be > merging this now. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#3119 (comment)>, or mute > the thread > < https://github.com./notifications/unsubscribe-auth/AH3QA2UWX2EEPUDWYR56Q23PRDUC7ANCNFSM4G622LCA > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLO3IVZN3IZRL466MZ5DPRDWSJANCNFSM4G622LCA> .

[phanisankar-nidadavolu]

I will do it myself but where should I place it, steps/data or would it still be in local directories? Phani On Thu, Apr 18, 2019 at 6:11 PM Daniel Povey <[email protected]> wrote:

…

It sounds like a great idea. Please, if you have time, either implement it yourself or work with the person who raised the issue to do so if you feel that is practical. On Thu, Apr 18, 2019 at 3:09 PM phanisankar-nidadavolu < ***@***.***> wrote: > Hello Dan, > > I think it is a good observation. musan corpus is originally at 16KHz. When > we use this script (musan.py) for augmenting 8k data (like swbd, sre16) we > downsample musan corpus to 8kHZ (like in the below line) > > utt2wav_str = utt2wav_str + utt + " sox -t wav " + utt2wav[utt] + " -r 8k > -t wav - |\n" > > if the corpus is at 16k (like SITW) this is not required. Ideal setup would > be to make sampling frequency optional and use same musan.py everywhere. > But since musan scripts are located in local directories, it is safe enough > to assume the sampling frequency is known in advance (you know whether to > resample it or not). Let me know what you think. > > Regarding merging I will push my latest changes tonight and we can merge. > > > Phani > > > On Thu, Apr 18, 2019 at 5:48 PM Daniel Povey ***@***.***> > wrote: > > > @david-ryan-snyder <https://github.com./david-ryan-snyder> or > > @phanisankar-nidadavolu <https://github.com./phanisankar-nidadavolu>, > > perhaps you can comment? And @phanisankar-nidadavolu > > <https://github.com./phanisankar-nidadavolu>, let me know if I should be > > merging this now. > > > > — > > You are receiving this because you were mentioned. > > Reply to this email directly, view it on GitHub > > <#3119 (comment)>, > or mute > > the thread > > < > https://github.com./notifications/unsubscribe-auth/AH3QA2UWX2EEPUDWYR56Q23PRDUC7ANCNFSM4G622LCA > > > > . > > > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#3119 (comment)>, or mute > the thread > < https://github.com./notifications/unsubscribe-auth/AAZFLO3IVZN3IZRL466MZ5DPRDWSJANCNFSM4G622LCA > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AH3QA2TGX4BJ3FWHXSPYFF3PRDWYFANCNFSM4G622LCA> .

[david-ryan-snyder]

The make_musan.py script is just a data prep scrip in local. Since SWBD uses audio sampled at 8kHz we know what the sample rate should be for this script. I don't see why this needs to be an option.

[danpovey]

Oh. Since we will likely be relying on this in the data augmentation scripts once we widely use them, I think it would make sense to put it somewhere shared though, e.g. steps/augmentation/ ?

…

On Thu, Apr 18, 2019 at 3:14 PM David Snyder ***@***.***> wrote: The make_musan.py script is just a data prep scrip in local. Since SWBD uses audio sampled at 8kHz we know what the sample rate should be for this script. I don't see why this needs to be an option. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLO2ZADTLKGLSKQLAXH3PRDXENANCNFSM4G622LCA> .

[david-ryan-snyder]

If we consider MUSAN important enough to put into steps/ it makes sense to change the usage so that the sample rate is an option. If you go that route, you should be sure to change all of the recipes that use MUSAN (that will include all of the x-vector recipes) so that it uses the new script in steps/ and also be sure to remove the old script in local.

[danpovey]

Agreed.

…

On Thu, Apr 18, 2019 at 3:16 PM David Snyder ***@***.***> wrote: If we consider MUSAN important enough to put into steps/ it makes sense to change the usage so that the sample rate is an option. If you go that route, you should be sure to change all of the recipes that use MUSAN (that will include all of the x-vector recipes) so that it uses the new script in steps/ and also be sure to remove the old script in local. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLO44ONBPXHNALKSKMT3PRDXNHANCNFSM4G622LCA> .

[david-ryan-snyder]

Moving it to steps/augmentation or steps/data sounds like a good idea to me.

[danpovey]

I prefer steps/augmentation/, as it's not a *generic* script for manipulating data dirs.

…

On Thu, Apr 18, 2019 at 3:18 PM David Snyder ***@***.***> wrote: Moving it to steps/augmentation or steps/data sounds like a good idea to me. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLO23CARQYVRDCD25E6DPRDXU7ANCNFSM4G622LCA> .

[danpovey]

Oh, I see, steps/data/ already has augmentation stuff. OK, put it there.

…

On Thu, Apr 18, 2019 at 3:21 PM Daniel Povey ***@***.***> wrote: I prefer steps/augmentation/, as it's not a *generic* script for manipulating data dirs. On Thu, Apr 18, 2019 at 3:18 PM David Snyder ***@***.***> wrote: > Moving it to steps/augmentation or steps/data sounds like a good idea to > me. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#3119 (comment)>, > or mute the thread > <https://github.com./notifications/unsubscribe-auth/AAZFLO23CARQYVRDCD25E6DPRDXU7ANCNFSM4G622LCA> > . >

[phanisankar-nidadavolu]

I will do that. Phani On Thu, Apr 18, 2019 at 6:22 PM Daniel Povey <[email protected]> wrote:

…

Oh, I see, steps/data/ already has augmentation stuff. OK, put it there. On Thu, Apr 18, 2019 at 3:21 PM Daniel Povey ***@***.***> wrote: > I prefer steps/augmentation/, as it's not a *generic* script for > manipulating data dirs. > > On Thu, Apr 18, 2019 at 3:18 PM David Snyder ***@***.***> > wrote: > >> Moving it to steps/augmentation or steps/data sounds like a good idea to >> me. >> >> — >> You are receiving this because you commented. >> Reply to this email directly, view it on GitHub >> <#3119 (comment)>, >> or mute the thread >> < https://github.com./notifications/unsubscribe-auth/AAZFLO23CARQYVRDCD25E6DPRDXU7ANCNFSM4G622LCA > >> . >> > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AH3QA2R7QRWQCGF2YGAPJ73PRDYC3ANCNFSM4G622LCA> .

[david-ryan-snyder]

@phanisankar-nidadavolu, is this PR done and ready to be reviewed?

[phanisankar-nidadavolu]

Yes it is. I ran a test run and the script has successfully reached TDNN training stage. Phani

…

On Mon, Apr 22, 2019 at 4:46 PM David Snyder ***@***.***> wrote: @phanisankar-nidadavolu <https://github.com./phanisankar-nidadavolu>, is this PR done and ready to be reviewed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AH3QA2XJJGJ6KYMNQGDPCT3PRYPZVANCNFSM4G622LCA> .

[david-ryan-snyder]

Is this comment still relevant?

[phanisankar-nidadavolu]

I think it is still relevant for now. We have not arrived at a general conclusion yet.

[david-ryan-snyder]

"original musal" -> "MUSAN"

[david-ryan-snyder]

Could you add a comment that says something like:

# The corpus can be cited as follows:
# @misc{musan2015,
#  author = {David Snyder and Guoguo Chen and Daniel Povey},
#  title = {{MUSAN}: {A} {M}usic, {S}peech, and {N}oise {C}orpus},
#  year = {2015},
#  eprint = {1510.08484},
#  note = {arXiv:1510.08484v1}
# }

[david-ryan-snyder]

I think this script should go in into a new tuning directory, e.g., local/chain/multi_style/tuning/run_tdnn_1a.sh.

Then, in local/chain/multi_style, I suggest creating a symbolic link to run_tdnn_1a.sh called run_tdnn.sh. That would be analogous to what is done in other recipes, such as https://github.com./kaldi-asr/kaldi/tree/master/egs/swbd/s5c/local/chain .

Also, I suggest adding a more detailed comment at the top of this script that also includes some WER results. For example, see https://github.com./kaldi-asr/kaldi/blob/master/egs/swbd/s5c/local/chain/tuning/run_tdnn_lstm_1n.sh.

[phanisankar-nidadavolu]

Makes sense but instead of creating the sym link in local/chain/multi_style it is better to create in local/chain/run_tdnn_multistyle.sh, since all the other tend scripts are located there. What do you think?

[david-ryan-snyder]

That sounds better.

[phanisankar-nidadavolu]

I made the changes except for the inclusion of WERs. I will include them once the tdnn finishes training. In my old experiments I never decoded on train_dev.

[danpovey]

Thanks David for the detailed review!

…

On Mon, Apr 22, 2019 at 7:33 PM David Snyder ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In egs/swbd/s5c/local/chain/multi_style/run_tdnn_1a.sh <#3119 (comment)>: > @@ -0,0 +1,256 @@ +#!/bin/bash I think this script should go in into a new tuning directory, e.g., local/chain/multi_style/tuning/run_tdnn_1a.sh. Then, in local/chain/multi_style, I suggest creating a symbolic link to run_tdnn_1a.sh called run_tdnn.sh. That would be analogous to what is done in other recipes, such as https://github.com./kaldi-asr/kaldi/tree/master/egs/swbd/s5c/local/chain . Also, I suggest adding a more detailed comment at the top of this script that also includes some WER results. For example, see https://github.com./kaldi-asr/kaldi/blob/master/egs/swbd/s5c/local/chain/tuning/run_tdnn_lstm_1n.sh . — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3119 (review)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLO5YKKZ6FOEORGSGVTTPRZDMNANCNFSM4G622LCA> .

[danpovey]

Yes, maybe having the symlink point from local/chain/ makes sense.

…

On Mon, Apr 22, 2019 at 8:48 PM phanisankar-nidadavolu < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In egs/swbd/s5c/local/chain/multi_style/run_tdnn_1a.sh <#3119 (comment)>: > @@ -0,0 +1,256 @@ +#!/bin/bash Makes sense but instead of creating the sym link in local/chain/multi_style it is better to create in local/chain/run_tdnn_multistyle.sh, since all the other tend scripts are located there. What do you think? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLOY6KYINDKAHMT2TRIDPRZMFFANCNFSM4G622LCA> .

[david-ryan-snyder]

I have two more comments on the aesthetics of the PR while you're waiting for the WERs.

I don't think we need the multi_style directory. This doesn't matter much for this particular PR, but as we start adding more scripts that use augmentation, deciding where these scripts will go will become more important. As I understand it, we plan on augmentation becoming part of the standard ASR recipe. I think it makes more sense to put theses scripts in the same place people are used to finding them, which is in chain/tuning.
My suggestion is to move the new script to local/chain/tuning/run_tdnn_multistyle_1a.sh and make local/chain/run_tdnn_multistyle.sh be a symbolic link to that.

Also, in my opinion, it's better to refer to these recipes using the word "augmentation" rather than "multi style." In my opinion, using some form of the word "augment" makes it immediately clear what you're doing, and then the terminology would be consistent with the new directory steps/data/augmentation that you're adding. Also, "multi style" sounds old fashioned to me. What do you think?

[david-ryan-snyder]

During the meeting today, @danpovey suggested that we use the word "aug" for the scripts in this recipe instead of "multi style" or "augmentation." Seems nice and concise, and everyone should know what that means at first glance, I think.

[vimalmanohar]

I think it is better to call it multi_condition or something to be consistent with existing recipes (in Aspire, AMI and Babel and probably many others) that already use that name for the same purpose. The scripts should go to local/multi_condition or local/chain/multi_condition/.

[david-ryan-snyder]

@vimalmanohar, my objection to this is because augmentation is now going to become a standard part of all or most of our ASR recipes. I think it's going to be cumbersome to propagate this terminology into the names of scripts for future recipes. If we need a way to distinguish between the past recipes without augmentation, and new recipes that use augmentation, adding the suffix "aug" to the new scripts is a concise (and I think, more intuitive) way to indicate what the change is.

If we have to go with multi_*, I agree multi_condition is better than multi_style since it already exists.

[vimalmanohar]

I think it is better to create a generic version of the script in steps like steps/copy_ali_dir.sh with option as --prefixes. It would be painful to keep copying this script to every new recipe.

[vimalmanohar]

Sem thing to this script. Also change ali-dir to lat-dir and give a description of what this script does.
seems like an inappropriate name as it is actually an input.

[vimalmanohar]

If clean data is also needed to be added, then add another option for this such as --include-original. This type of option is already used in one of the older scripts.

[vimalmanohar]

This script should be also be called local/nnet3/multi_condition/run_ivector_common.sh. It is same as the one that is in aspire, AMI and Babel but uses a new script for augmentation.

[phanisankar-nidadavolu]

We have made use_ivectors an optional argument. I am not sure run_ivector_common.sh is a valid name in that case. We either have to remove use_ivectors flag and make training of ivectors a default option or name the script prepare_aug_data.sh. Let me know what you think.

[danpovey]

Hm, maybe prepare_aug_data.sh is OK then. We may not end up using ivectors in all cases. I don't have super strong opinoins about that.

[danpovey]

Maybe run_aug_common.sh?

[vimalmanohar]

I think if this script is being created new, it would be better to stick to the PEP8 standards, which includes using function names like get_args() instead of GetArgs().
But more importantly I think some of the things like imp.load_source may not work in python3. Or at least, it is very old style and should be modified to use import data_dir_manipulation_lib as data_lib.

[vimalmanohar]

I think instead of assuming this exists, it is better to first call utils/data/get_reco2dur.sh. Otherwise this is going to be called once for each augmentation that will be done.

[phanisankar-nidadavolu]

Hi Vimal, the wrapper script prepare_multistyle_data.sh (now changed to run_ivector_aug.sh) first creates reco2dur file in the input directory and then we call this script to augment. I guess it is safe to assume that this file exists (at least for this recipe). Do you want me to modify the script to first check whether the file exists or not, and create the file if it does not exist?

[vimalmanohar]

You can add it do this script. utils/data/get_reco2dur.sh already will check if file exists and create it only if not. So you can simply call that script.

[vimalmanohar]

If this is removed, then the script will fail if the output_dir exists.

[phanisankar-nidadavolu]

@vimalmanohar I don't see why. This part of the code is only creating output dir if it does not exist. Anyways, I am modifying the code to fail if the output directory already exists.

[danpovey]

I don't think Vimal was asking for it to fail if the output dir exists (and I don't recommend it either).
I think he's pointing out that since, below, you do os.makedirs with no such guard, it will fail if it did not
exist. It seems to have an exist_ok parameter which you can set to true.

[vimalmanohar]

The original version of the script will re-create the augmented data dir if the script is run again. But Phani modified it so it would do nothing if run again, which I think is unexpected behavior. It should either 1) re-create the augmented data dir, 2) fail with error saying it already exists.

[vimalmanohar]

I don't think it should skip creating it if its already there. I think it should either recreate it or fail with an error saying it already exists.

[danpovey]

I think aug / augmentation is a better name because eventually we will likely include other augmentation methods in this setup. and multi_condition is a mouthful when used in directory names.

…

On Thu, Apr 25, 2019 at 9:13 PM Vimal Manohar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In egs/swbd/s5c/local/copy_ali_dir.sh <#3119 (comment)>: > @@ -0,0 +1,60 @@ +#!/bin/bash + +noise_list="reverb1:babble:music:noise" +max_jobs_run=50 I think it is better to create a generic version of the script in steps like steps/copy_ali_dir.sh with option as --prefixes. It would be painful to keep copying this script to every new recipe. ------------------------------ In egs/swbd/s5c/local/copy_lat_dir.sh <#3119 (comment)>: > @@ -0,0 +1,60 @@ +#!/bin/bash + +noise_list="reverb1:babble:music:noise" +max_jobs_run=50 +nj=100 +cmd=queue.pl +write_compact=true + +. ./path.sh +. utils/parse_options.sh + +if [ $# -ne 3 ]; then + echo "Usage: $0 <out-data> <src-ali-dir> <out-ali-dir>" Sem thing to this script. Also change ali-dir to lat-dir and give a description of what this script does. seems like an inappropriate name as it is actually an input. ------------------------------ In egs/swbd/s5c/local/copy_lat_dir.sh <#3119 (comment)>: > +rm -f $dir/lat_tmp.*.{ark,scp} 2>/dev/null + +# Copy the alignments temporarily +echo "creating temporary lattices in $dir" +$cmd --max-jobs-run $max_jobs_run JOB=1:$num_jobs $dir/log/copy_lat_temp.JOB.log \ + lattice-copy --write-compact=$write_compact \ + "ark:gunzip -c $src_dir/lat.JOB.gz |" \ + ark,scp:$dir/lat_tmp.JOB.ark,$dir/lat_tmp.JOB.scp || exit 1 + +# Make copies of utterances for perturbed data +utt_prefixes=`echo $noise_list | awk -F ":" '{for (i=1; i<=NF; i++) printf "%s- ", $i}'` +for p in $utt_prefixes; do + cat $dir/lat_tmp.*.scp | awk -v p=$p '{print p$0}' +done | sort -k1,1 > $dir/lat_out.scp.noise + +cat $dir/lat_tmp.*.scp | awk '{print $0}' | sort -k1,1 > $dir/lat_out.scp.clean If clean data is also needed to be added, then add another option for this such as --include-original. This type of option is already used in one of the older scripts. ------------------------------ In egs/swbd/s5c/local/nnet3/prepare_multistyle_data.sh <#3119 (comment)>: > @@ -0,0 +1,225 @@ +#!/bin/bash +# Copyright 2019 Phani Sankar Nidadavolu +# Apache 2.0. + +. ./cmd.sh + +set -e +stage=0 This script should be also be called local/nnet3/multi_condition/run_ivector_common.sh. It is same as the one that is in aspire, AMI and Babel but uses a new script for augmentation. ------------------------------ In egs/wsj/s5/steps/data/augment_data_dir.py <#3119 (comment)>: > from reverberate_data_dir import ParseFileToDict from reverberate_data_dir import WriteDictToFile +import libs.common as common_lib data_lib = imp.load_source('dml', 'steps/data/data_dir_manipulation_lib.py') I think if this script is being created new, it would be better to stick to the PEP8 standards, which includes using function names like get_args() instead of GetArgs(). But more importantly I think some of the things like imp.load_source may not work in python3. Or at least, it is very old style and should be modified to use import data_dir_manipulation_lib as data_lib. ------------------------------ In egs/wsj/s5/steps/data/augment_data_dir.py <#3119 (comment)>: > input_dir = args.input_dir output_dir = args.output_dir + + if os.path.exists(output_dir): + data_lib.RunKaldiCommand("utils/fix_data_dir.sh {output_dir}".format( + output_dir = output_dir)) + print("directory {} already exists, not creating it again".format(output_dir)) + sys.exit() + + fg_snrs = [int(i) for i in args.fg_snr_str.split(":")] + bg_snrs = [int(i) for i in args.bg_snr_str.split(":")] num_bg_noises = [int(i) for i in args.num_bg_noises.split(":")] reco2dur = ParseFileToDict(input_dir + "/reco2dur", I think instead of assuming this exists, it is better to first call utils/data/get_reco2dur.sh. Otherwise this is going to be called once for each augmentation that will be done. ------------------------------ In egs/wsj/s5/steps/data/reverberate_data_dir.py <#3119 (comment)>: > @@ -84,9 +84,6 @@ def GetArgs(): return args def CheckArgs(args): - if not os.path.exists(args.output_dir): If this is removed, then the script will fail if the output_dir exists. ------------------------------ In egs/wsj/s5/steps/data/reverberate_data_dir.py <#3119 (comment)>: > @@ -653,6 +658,12 @@ def Main(): pointsource_noise_addition_probability = args.pointsource_noise_addition_probability, max_noises_per_minute = args.max_noises_per_minute) + else: + print("Directory {0} already exists, not creating it again".format(args.output_dir)) I don't think it should skip creating it if its already there. I think it should either recreate it or fail with an error saying it already exists. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (review)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLOZMEDC3JJNVWWQ72LDPSJJMLANCNFSM4G622LCA> .

[vimalmanohar]

I guess it makes sense then to use augmentation if all the new recipes have it. But perhaps it is better to have the new recipes in local/chain/augmentation/tuning and then create a symlink in local/chain or local/chain/augmentation.

[danpovey]

I'd rather not add a new thing in the hierarchy if this is going to become the default. The idea is to have this be the normal recipe in local/chain/run_tdnn.sh. It might have to call a different run_ivector_common.sh script though.

…

On Thu, Apr 25, 2019 at 9:21 PM Vimal Manohar ***@***.***> wrote: I guess it makes sense then to use augmentation if all the new recipes have it. But perhaps it is better to have the new recipes in local/chain/augmentation/tuning and then create a symlink in local/chain or local/chain/augmentation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AAZFLO5UU66QFRTD2DC2CFLPSJKLBANCNFSM4G622LCA> .

[phanisankar-nidadavolu]

Hmm. Got it. Phani

…

On Wed, May 1, 2019 at 1:41 PM Daniel Povey ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In egs/wsj/s5/steps/data/reverberate_data_dir.py <#3119 (comment)>: > @@ -84,9 +84,6 @@ def GetArgs(): return args def CheckArgs(args): - if not os.path.exists(args.output_dir): I don't think Vimal was asking for it to fail if the output dir exists (and I don't recommend it either). I think he's pointing out that since, below, you do os.makedirs with no such guard, it will fail if it did not exist. It seems to have an exist_ok parameter which you can set to true. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AH3QA2W4KNLOAIXGNQ26X6LPTHI33ANCNFSM4G622LCA> .

[phanisankar-nidadavolu]

run_aug_common.sh is good. Phani

…

On Wed, May 1, 2019 at 1:30 PM Daniel Povey ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In egs/swbd/s5c/local/nnet3/prepare_multistyle_data.sh <#3119 (comment)>: > @@ -0,0 +1,225 @@ +#!/bin/bash +# Copyright 2019 Phani Sankar Nidadavolu +# Apache 2.0. + +. ./cmd.sh + +set -e +stage=0 Maybe run_aug_common.sh? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3119 (comment)>, or mute the thread <https://github.com./notifications/unsubscribe-auth/AH3QA2UPDFCTBGRNIRCLFYLPTHHURANCNFSM4G622LCA> .

[phanisankar-nidadavolu]

Hello, the PR looks good with changes that you suggested. Main changes made: renaming to aug, moving around files, adding results and some other stuff.