search

amaurycrickx / recognito

Java Speaker Recognition Framework
Apache License 2.0
191 stars 102 forks source link

Updated Chebyshev distance test & syntax highlight for readme #5

Closed brianchung808 closed 10 years ago

amaurycrickx commented 10 years ago

Hi Brian,

I don't really understand the need for "Add methods for setting feature vectors through double[]"

Here is my view:

  1. The user shouldn't care nor be exposed to the inner details of the lib.
  2. I'm not convinced an array of doubles will always be the one and only thing required to specify a VoicePrint

Basically, these are the reasons why VoicePrint has no public constructor (only package visibility). I'm afraid I can't think of a good reason to change my mind...

I hope you don't mind but I'd also like to add a note on design here... Maybe I'm wrong but I'm under the impression you added these methods because there was no public constructor on VoicePrint. I'd say copy/pasting entire methods with a single line changed is usually a bad smell. Since you are circumventing the restriction on VoicePrint's constructor anyway, why didn't you make it public ?

brianchung808 commented 10 years ago

Oh sorry the rest of the commits were after I sent the pull request. I only meant to include the syntax highlighting for readme and the chebyshev test update. The rest should not have been included.

amaurycrickx commented 10 years ago

No prob :-)

amaurycrickx commented 10 years ago

Since the changes were that small, I was willing to copy them manually. I wonder how it's possible you run the test and get 19.0 where I get 200.0 !?!

brianchung808 commented 10 years ago

Hmm it should be 19.0 with my last pull req fixing the Chebyshev distance calculation. I get 19.0.

amaurycrickx commented 10 years ago

You're damn right ! 19 ... Should have used my brains and check for myself :-)

Hadn't performed a clean build in Eclipse after merge and the compiled code used when running the test was still the old one...

Another reason I should be setting up Jenkins