bug: ivector-bin/agglomerative-cluster.cc

[dsmiller]

The following snippet from ivector-bin/agglomerative-cluster.cc assigns threshold to its negative after reading in each recording. So threshold toggles from, e.g., 0.1, -0.1, 0.1, etc. Also, because of the missing brackets for the if statement, it will get flipped regardless of read_costs.

I didn't do a PR because it's not clear if threshold's sign should be flipped once (outside of the scores reader block) or if threshold should be specified on the command line appropriately for the type of scores to read. I can do a quick PR if someone clarifies the intent here.

for (; !scores_reader.Done(); scores_reader.Next()) {  
      std::string reco = scores_reader.Key(); 
      Matrix<BaseFloat> costs = scores_reader.Value();   
      // By default, the scores give the similarity between pairs of
      // utterances.  We need to multiply the scores by -1 to reinterpet
      // them as costs (unless --read-costs=true) as the agglomerative
      // clustering code requires.
      if (!read_costs)
        costs.Scale(-1);
        threshold = -threshold;
    ...
}

[david-ryan-snyder]

Thanks @dsmiller! I think --read-costs=true is how we usually run this, so this didn't break things earlier.

@mmaciej2, can you please make a PR to fix this? scratch that, since @dsmiller wants to handle it.

[david-ryan-snyder]

think the intent was for

if (!read_costs) threshold = -threshold;

To be moved before for (; !scores_reader.Done(); scores_reader.Next()). As you said, it doesn't make sense that this threshold negating is happening in the loop.

Could you go ahead and make that change, and make a PR for it? Please add @david-ryan-snyder to the description of the PR so that I see it.

[david-ryan-snyder]

Thanks!