BLEU score #2477
BLEU score #2477
Conversation
| } | ||
|
|
||
| std::map<WordVector, size_t> translationNGramCounts = GetNGrams(*trIt); | ||
| std::map<WordVector, size_t> overlap; |
There was a problem hiding this comment.
We can use boost::string_view instead of std::string here and many other place too
There was a problem hiding this comment.
I've used
typedef typename TranslationCorpusType::value_type WordVector;statement in line number 52. I think it's up to the user what to be used.
There was a problem hiding this comment.
It is ok to give user a choice, but here overlap variable doesn't have any relation with the user, I mean it is not exposed to user
map<<vector<string>>, size_t> overlap , it would be unnecessary copy of whole string when inserting the value in overlap map
That's my POV, may be you can wait for someone to give a better insight
There was a problem hiding this comment.
But I think it will be better to add tests for both types :)
There was a problem hiding this comment.
it would be unnecessary copy of whole string when inserting the value in overlap map
Ok I will see this. Thanks for clarifying.
|
@lozhnikov If you get some time, can you look this PR. This is done from my side. |
|
@lozhnikov @zoq Can this be merged now? |
I think it's almost done. Let me look at the code a couple of times. |
mrityunjay-tripathi
force-pushed
the
bleu_score
branch
from
8b18226
to
92f9adf
Compare
|
@lozhnikov Can we conclude this PR? |
Co-authored-by: Mikhail Lozhnikov <lozhnikovma@gmail.com>
![mlpack-bot[bot]](https://avatars.githubusercontent.com/in/23934?s=60&v=4){kind=small}
|
I merged the PR. Thanks for the contribution! |
|
I thought the App Veyor build failed due to time limit. In fact it doesn't like the conversion from |
|
Thanks, @lozhnikov for all the reviews and suggestions:) Glad that you found the issue really quick. |
No description provided.