search

OpenNMT / CTranslate

Lightweight C++ translator for OpenNMT Torch models (deprecated)
https://opennmt.net/
MIT License
79 stars 50 forks source link

add n-best feature #37

Closed jhnwnd closed 6 years ago

jhnwnd commented 6 years ago

You're welcome and thank you for all your work and for reviewing this. Yes, I verified that you get the same output as before with n_best=1, although in the current master code there is no check to ensure that an EOS has no children except for if the EOS first occurs on the top of the beam - it looks like an oversight: there is a boolean variable in the current master Translator.hxx called found_eos that is being set but never being checked. It does seem to not matter (I was unable to prove this, maybe you can enlighten me), but I did incorporate that into this change. I believe that is the only significant difference between how this code works with n_best=1 and the previous code. Requested change for throwing an exception done.

guillaumekln commented 6 years ago

Yes, I think we overlooked found_eos because we only care about the top of the beam. Thanks for this PR!

jhnwnd commented 6 years ago

One more thing: the simple example on README.md is no longer correct. std::cout << translator->translate("Hello world !") << std::endl; should be std::cout << translator->translate("Hello world !")[0] << std::endl; unless you prefer to overload translate to keep the example working.

guillaumekln commented 6 years ago

Indeed, this PR is breaking the API. If you have time, maybe you could keep the existing translate methods and implement new ones such as translate_with_hypotheses (we can't overload with just the return type so another name is required).

jhnwnd commented 6 years ago

Sure, see PR #41