kaldi-asr / kaldi

kaldi-asr/kaldi is the official location of the Kaldi project.
http://kaldi-asr.org
Other
14.25k stars 5.32k forks source link

copy-vector --scale doesn't work #4494

Closed danijel3 closed 3 years ago

danijel3 commented 3 years ago

I'm not sure if I'm crazy, but this seems like a pretty basic bug: The --scale and --change-dim options don't work in the "copy-vector" program. To test it, I run the following code:

$ echo 'test [ 1 2 3 4 ]' | copy-vector --scale=2 ark,t:- ark,t:-
copy-vector --scale=2 ark,t:- ark,t:- 
test  [ 1 2 3 4 ]
LOG (copy-vector[5.5.891~1-37628]:main():copy-vector.cc:98) Copied 1 vectors, setting dim to -1 scaled by 2

The correct output should be obviously:

test  [ 2 4 6 8 ]

The error is in this line: https://github.com/kaldi-asr/kaldi/blob/58363408a8efc78d896ab2cc83d215cac314b4e6/src/bin/copy-vector.cc#L96

The existing line:

writer.Write(reader.Key(), reader.Value());

Should be:

writer.Write(reader.Key(), vec);

Now how can it be that such a basic error went under the radar for this many years? I looked for all uses of copy-vector in egs and it seems that scale wasn't used once. I'm leaving this as an issue, because it may raise questions on some other bugs that I'm not aware of.

kkm000 commented 3 years ago

Thanks for digging into it! Could you send a PR with a fix please?

kkm000 commented 3 years ago

@danpovey, we have vector-scale which is used in recipes, and copy-vector which is never used with either of the two switches. The fixed version should likely supersede vector-scale, since it's more powerful. We can check in an executable shell script named vector-scale that just execs copy-vector, or a softlink.

Is these something I'm missing?

kkm000 commented 3 years ago

@danpovey , correction, copy-vector --change-dim is used. In steps/train_sgmm2.sh and steps/train_sgmm2_group.sh which are dead wood by now.

kkm000 commented 3 years ago

X-ref: fixed in #4515.

Thanks!