Add the "fill_mode" option in augment-image.cc by YiwenShaoStephen · Pull Request #1602 · kaldi-asr/kaldi

YiwenShaoStephen · 2017-05-02T11:58:09Z

No description provided.

…ure python

danpovey · 2017-05-03T23:09:56Z

src/nnet3bin/nnet3-egs-augment-image.cc

 namespace kaldi {
 namespace nnet3 {

+enum fill_mode_type {kNearest, kReflect};


space after {

danpovey · 2017-05-03T23:10:34Z

src/nnet3bin/nnet3-egs-augment-image.cc

+	  if (r2 < 0) r2 = - r2;
+	}
+	if (r2 >= num_rows) {
+	  r2 = num_rows -1 - (r2 - num_rows + 1);


I think you could simplify this expression.

No problem. I didn't simplify it just for ease of understanding.

danpovey · 2017-05-03T23:11:03Z

src/nnet3bin/nnet3-egs-augment-image.cc


    ParseOptions po(usage);
    po.Register("srand", &srand_seed, "Seed for the random number generator");
+    po.Register("fill_mode", &fill_mode_string, "Mode for filling the out-bundaries points = {nearest, reflect}");


use "fill-mode"
typo: bundaries should be boundaries;
and try to stay within the google-style-guide-mandated 80-character limit. e.g.

po.Register("fill-mode", &fill_mode_string, "Mode for dealing with " "points outside the image boundary when applying transformations. " "Choices = {nearest, reflect}");

I will fix it.

danpovey · 2017-05-03T23:12:58Z

src/nnet3bin/nnet3-egs-augment-image.cc

+    fill_mode_type fill_mode;
+    if (fill_mode_string == "reflect"){
+      fill_mode = kReflect;
+    } else {


add:

if (fill_mode_string != "nearest") KALDI_ERR << "Choices for --fill-mode are 'nearest' or 'reflect', got: " << fill_mode_string;

hhadian · 2017-05-04T16:34:54Z

src/nnet3bin/nnet3-egs-augment-image.cc

 namespace kaldi {
 namespace nnet3 {

+enum fill_mode_type { kNearest, kReflect};


According to Google style, enum name should be in first-letter-capital format: i.e. FillModeType or simply FillMode.

Also there should be a space before }

hhadian · 2017-05-04T16:52:59Z

src/nnet3bin/nnet3-egs-augment-image.cc

                          int32 num_channels,
-                          MatrixBase<BaseFloat> *image) {
+                          MatrixBase<BaseFloat> *image,
+			  fill_mode_type fill_mode) {


I can see you are using tabs. You need to change it to spaces. Fix other places in your code too.

hhadian · 2017-05-04T16:57:39Z

src/nnet3bin/nnet3-egs-augment-image.cc

      po.PrintUsage();
      exit(1);
    }
+


There is a trailing space here. Please fix here and other places and configure your editor to disallow trailing spaces.

hhadian · 2017-05-04T17:33:54Z

src/nnet3bin/nnet3-egs-augment-image.cc

+	  c1 = - c1;
+	  if (c2 < 0) c2 = -c2;
+	}
+	if (c2 >= num_cols) {


All these num_cols should be changed to height. This issue existed in my code to. Please fix them as well.
Maybe better to rename num_rows to width to avoid future confusion.

hhadian · 2017-05-04T21:38:48Z

src/nnet3bin/nnet3-egs-augment-image.cc


    ParseOptions po(usage);
    po.Register("srand", &srand_seed, "Seed for the random number generator");
+    po.Register("fill-mode", &fill_mode_string, "Mode for dealing with "


Shouldn't this option be defined in the ImageAugmentationConfig class? Is it here because it is a string and is converted to Enum later?

danpovey · 2017-05-04T21:47:05Z

You're right, it should be in that class. It's OK to turn it to an enum inside the function, as long as it's outside of any loops.

…

On Thu, May 4, 2017 at 5:38 PM, Hossein Hadian ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/nnet3bin/nnet3-egs-augment-image.cc <#1602 (comment)>: > ImageAugmentationConfig config; ParseOptions po(usage); po.Register("srand", &srand_seed, "Seed for the random number generator"); + po.Register("fill-mode", &fill_mode_string, "Mode for dealing with " Shouldn't this option be defined in the ImageAugmentationConfig class? Is it here because it is a string and is converted to Enum later? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1602 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVu0pxSYD7xHqhcULZZhEJzumbWp6Bks5r2kVrgaJpZM4NODCe> .

danpovey · 2017-05-04T21:47:55Z

... or have a function FillMode GetFillMode() const; in the augmentation-config class, that works out the fill-mode as an enum.

…

On Thu, May 4, 2017 at 5:47 PM, Daniel Povey ***@***.***> wrote: You're right, it should be in that class. It's OK to turn it to an enum inside the function, as long as it's outside of any loops. On Thu, May 4, 2017 at 5:38 PM, Hossein Hadian ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/nnet3bin/nnet3-egs-augment-image.cc > <#1602 (comment)>: > > > > ImageAugmentationConfig config; > > ParseOptions po(usage); > po.Register("srand", &srand_seed, "Seed for the random number generator"); > + po.Register("fill-mode", &fill_mode_string, "Mode for dealing with " > > Shouldn't this option be defined in the ImageAugmentationConfig class? Is > it here because it is a string and is converted to Enum later? > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#1602 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ADJVu0pxSYD7xHqhcULZZhEJzumbWp6Bks5r2kVrgaJpZM4NODCe> > . >

danpovey · 2017-05-05T19:36:07Z

Thanks a lot! Merging.

…#1602)

YiwenShaoStephen added 8 commits April 25, 2017 07:14

finish the script to transform matrix to png image for debugging

5eee127

fix bugs in matrix_to_image.py not working with different matrix format

4653f06

merge

51cca86

finish the bmp_encoder.py and enable matrix_to_image.py to run with p…

e934a65

…ure python

Merge remote-tracking branch 'official/kaldi_52' into kaldi_52

92242d1

Enable the fill_mode option

3bbc674

set default mode to nearest

99a87e5

fix the compilation error in augment-image

c7e3ae1

danpovey reviewed May 3, 2017

View reviewed changes

do modifications in augment-image

0101cf0

hhadian reviewed May 4, 2017

View reviewed changes

Further modification in augment-image

ee76f55

danpovey merged commit 13b08e4 into kaldi-asr:kaldi_52 May 5, 2017

Skaiste pushed a commit to Skaiste/idlak that referenced this pull request Sep 26, 2018

[src] Add --fill-mode option in nnet3-egs-augment-image.cc (kaldi-asr…

337e355

…#1602)

Conversation

YiwenShaoStephen commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danpovey commented May 4, 2017 via email

Uh oh!

danpovey commented May 4, 2017 via email

Uh oh!

danpovey commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants