Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use priority queue to find n-best sequences #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rsennrich
Copy link

the current implementation (lines 369-378) has a complexity of O(n**2), whereas the priority queue should work in O(n log n).

with a label set of size 50, n-best tagging was about the same speed up to n=10; for n=50, the new implementation was about 6x faster in a small benchmark.

@rsennrich
Copy link
Author

just found some additional overhead (the code now keeps only Y hypotheses in the priority queue, since we know that for each previous hypothesis, the n-best list is already sorted). Now 50-best tagging is about 20-30x faster than the original implementation.

@Jekub
Copy link
Owner

Jekub commented Apr 2, 2013

Thanks for this contribution, it looks cool. Can you please just update your code to match the style of wapiti code. I want to enforce consistent style in all the code base.
I will do some testing later this week before merging.

@ghost ghost assigned Jekub Apr 2, 2013
@rsennrich
Copy link
Author

do you mean the heap.c and heap.h ? Those aren't mine, but from https://github.com/willemt/CHeap (BSD license). The downside would be that changing the code style will make it harder to merge in any fixes/optimizations from/to upstream (although the implementation looks robust).

@rsennrich
Copy link
Author

just a quick follow-up because I haven't heard back from you regarding the last comment - do you also want to enforce the code style for external code (such as the heap implementation mentioned above)?

@arjunmenon
Copy link

@rsennrich Hey is it compatible with the new version of CHeap? There are many variables, definition changes in the new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants