Skip to content

Conversation

@michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Oct 22, 2025

This PR returns the behavior of the ScoreTracker to before #501. There are two issues:

  1. The RelaxedMonotonicityTracker had a fixed size bestScores at new BoundedLongHeap(100). The previous behavio used a size that varied based on the rerankK for the query. Further, when the reset method is called, we do not update the size, and as such, we end up with a mismatched bestScores heap, which can result in an incorrect understanding of the worstBestScore in the shouldStop method. In cases where the rerankK is lower than 100, we will search deeper into the graph while in cases where it is above 100, we may return early.
  2. The ScoreTrackerFactory#getScoreTracker has enough information to initialize more correct versions of the trackers, so I do that.

@github-actions
Copy link
Contributor

Before you submit for review:

  • Does your PR follow guidelines from CONTRIBUTIONS.md?
  • Did you summarize what this PR does clearly and concisely?
  • Did you include performance data for changes which may be performance impacting?
  • Did you include useful docs for any user-facing changes or features?
  • Did you include useful javadocs for developer oriented changes, explaining new concepts or key changes?
  • Did you trigger and review regression testing results against the base branch via Run Bench Main?
  • Did you adhere to the code formatting guidelines (TBD)
  • Did you group your changes for easy review, providing meaningful descriptions for each commit?
  • Did you ensure that all files contain the correct copyright header?

If you did not complete any of these, then please explain below.

@michaeljmarshall michaeljmarshall self-assigned this Oct 22, 2025
@eolivelli
Copy link

which unit tests cover this change ?

Copy link
Contributor

@marianotepper marianotepper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch. Thanks for the contribution!

@michaeljmarshall
Copy link
Member Author

@eolivelli - I am adding tests this morning. I found the fix at the very end of my day yesterday and wanted to push something up, though as we can see, it wasn't a global fix since I used the wrong value for the setMaxSize value.

@marianotepper marianotepper self-requested a review October 22, 2025 15:20
Copy link
Contributor

@marianotepper marianotepper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jshook jshook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marianotepper
Copy link
Contributor

Added a unit test in JV that revealed the bug

@michaeljmarshall
Copy link
Member Author

Thanks for adding the test @marianotepper!

@marianotepper marianotepper merged commit d95bc61 into main Oct 22, 2025
31 of 39 checks passed
@marianotepper marianotepper deleted the cndb-15703 branch October 22, 2025 18:32
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.

5 participants