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

Fix finalize error in ContextGraph implementation. #1443

Closed
wants to merge 1 commit into from

Conversation

maxwellzh
Copy link

For Aho-Corasicks algorithm, at the end of beam search, we should subtract scores of edges state.node_score only if current state is not an end of word.
Fix is simple.
cc @pkufool

@pkufool
Copy link
Collaborator

pkufool commented Dec 29, 2023

@maxwellzh Thanks!, but the fix is not correct. I have some major improvements in this PR #1428 .

@maxwellzh
Copy link
Author

@pkufool plz correct me if I'm wrong,

  1. node.node_score is the cumulative bonus of all edges from root to current node. This score is used to encourage beam search going along word token path
  2. And there is an output_score as bonus for word completion. This score encourages beam search to get completed word.

So at finalize(), why you would like to subtract all bonus of edges for a completed word?

e.g.
For a graph with single edge (1, 2, 3), weight 1.0

At finalize(), path [1,2,3] is expected to have 1.0+1.0 bonus, which without the fix would be 1.0 instead. Or is this by design?

@pkufool
Copy link
Collaborator

pkufool commented Dec 31, 2023

@maxwellzh Sorry, I am not quite follow your example. The idea is in current implementation all bonus are contained in the node.output_score already, you can have a look at the test cases in context_graph.py, it will draw the graph into pdf format, you can go through all cases step by step.

@maxwellzh
Copy link
Author

@pkufool Sry I misunderstood the implementation. Thanks for the clarification :)

@maxwellzh maxwellzh closed this Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants