fix(mps): make bisection_timestamp_search return the truly closest index - #354
Closed
Ace3Z wants to merge 2 commits into
Closed
fix(mps): make bisection_timestamp_search return the truly closest index#354Ace3Z wants to merge 2 commits into
Ace3Z wants to merge 2 commits into
Conversation
Ace3Z
force-pushed
the
fix/bisection-timestamp-closest-neighbor
branch
2 times, most recently
from
May 25, 2026 08:51
6bbc782 to
0805945
Compare
The binary search converged to whichever side it happened to land on, not the index with the smallest absolute distance to the query. For example with timestamps [1000, 3000, ..., 19000] and query 14900 it returned index 6 (distance 1900) instead of 7 (distance 100). The four get_nearest_* wrappers all advertise "closest or equal" in their docstrings, so they were silently returning the wrong neighbor for any query that fell between samples. After the existing loop, compare the converged index with its neighbors (start - 1, start, start + 1) and return whichever has the smallest absolute distance; ties resolve to the lower index. Closes facebookresearch#100. Credit @georgegu1997 for the analysis in the issue thread.
… timestamp exactly The border guards used <= and >=, so querying the exact first or last timestamp returned None instead of index 0 or len - 1, violating the "closest or equal" wording in every get_nearest_* wrapper docstring. Tightened to < and >. Separable from the issue facebookresearch#100 fix; can be dropped if you'd rather see it as its own PR.
Ace3Z
force-pushed
the
fix/bisection-timestamp-closest-neighbor
branch
from
May 25, 2026 10:20
0805945 to
1d7e6b3
Compare
Contributor
Author
|
Friendly ping @SeaOtocinclus. Following up on #100 where you suggested opening a PR for integration. This implements the fix with unit tests wired into CI. Happy to adjust anything if needed. |
Contributor
Author
|
Friendly ping. @YLouWashU, would you have a moment to take a look? Small one line fix to make |
Contributor
|
@SeaOtocinclus has imported this pull request. If you are a Meta employee, you can view this in D107104515. |
Contributor
|
@SeaOtocinclus merged this pull request in 9b4e915. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #100.
bisection_timestamp_searchwas supposed to return the closest timestamp but it returned whichever side the binary search happened to land on. With[1000, 3000, ..., 19000]and query 14900, it returned index 6 (distance 1900) instead of 7 (distance 100). All fourget_nearest_*wrappers advertise "closest or equal" in their docstrings, so they were silently returning the wrong neighbor for any query that fell between samples.Credit to @georgegu1997 for the analysis in the issue thread.
Structured as two commits in case the second one feels out of scope.
Commit 1 keeps the existing border guards and binary search loop untouched. After the loop converges, the new code compares the three candidate indices (
start - 1,start,start + 1) by absolute distance and returns the closest one. Ties break toward the lower index.Commit 2 is a separate but related bug that fell out of the same review. The original border guards used
<=and>=, so querying the exact first or last timestamp returned None instead of index 0 or len-1. Tightened to<and>. Happy to drop this commit if you'd rather see it as its own PR.Added 3 unittests in
core/python/test/mpsUtilsTest.py(same naming and style asmpsPyBindTest.py), wired intobuild-and-test.yml. On main all three fail (the #100 regression, the tie-break, and the endpoint case). On commit 1 alone the endpoint case still fails. On the full PR they all pass. The tests useSimpleNamespaceto stub thetracking_timestampattribute so they run withoutpip install .; CI still does the full install so the real import path is exercised.All four internal callsites of
bisection_timestamp_searchjust dodata[bisection_index]after a None check; none depend on the old "always less or equal" behavior.