(refac!): Remove HintedBinary, route Binary+hint to LinearBinary#45
Merged
(refac!): Remove HintedBinary, route Binary+hint to LinearBinary#45
HintedBinary, route Binary+hint to LinearBinary#45Conversation
BREAKING: Delete HintedBinary struct, _search_hinted_binary!, and all
HintedBinary dispatch overloads. Binary()+hint and AutoSearch()+hint
now auto-upgrade to LinearBinary{2} instead.
Add LinearBinary(linear_window=0) as the direct replacement for
HintedBinary behavior (hint check only, no walk, binary fallback).
Replace all HintedBinary references across 8 test files:
- Searcher{HintedBinary,RefHint} → Searcher{LinearBinary{0},RefHint}
- search=HintedBinary() → search=LinearBinary(linear_window=0)
- Auto-upgrade assertions: now expect LinearBinary{2} not HintedBinary
- Delete _search_hinted_binary! direct coverage test (function removed)
- Fix LinearBinary constructor test: 0 is now a valid linear_window
- Delete HintedBinary section from policies.md - Remove from decision matrix and @docs block - Update auto-upgrade docs: Binary+hint → LinearBinary{2} - Add linear_window=0 to LinearBinary tuning guide
There was a problem hiding this comment.
Pull request overview
Removes the HintedBinary search policy and consolidates all hint-based binary behavior under LinearBinary, including enabling LinearBinary(linear_window=0) as the direct replacement and routing Binary()/AutoSearch() + hint auto-upgrades to LinearBinary() (default window).
Changes:
- Delete
HintedBinaryand its internal search implementation; remove from exports/show formatting. - Allow
LinearBinary(linear_window=0)and use it as the functional replacement forHintedBinary. - Update tests and docs to replace
HintedBinaryreferences and adjust auto-upgrade expectations.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/core/search.jl |
Removes HintedBinary, adds linear_window=0 support, and reroutes Binary/AutoSearch hint upgrades to LinearBinary() |
src/core/show.jl |
Removes _format_search(::HintedBinary) formatting |
src/FastInterpolations.jl |
Drops HintedBinary from exports |
src/linear/linear_oneshot.jl |
Updates docstrings to reference LinearBinary(linear_window=0) instead of HintedBinary() |
src/quadratic/quadratic_oneshot.jl |
Updates docstrings to reference LinearBinary(linear_window=0) instead of HintedBinary() |
src/constant/constant_oneshot.jl |
Updates docstrings to reference LinearBinary(linear_window=0) instead of HintedBinary() |
src/cubic/cubic_types.jl |
Updates type documentation to remove HintedBinary mention |
test/test_search.jl |
Replaces HintedBinary usage with LinearBinary{0} equivalents; updates constructor validity tests and auto-upgrade assertions |
test/test_show.jl |
Updates show-format expectations from HintedBinary to LinearBinary{0} |
test/test_thread_safety.jl |
Updates concurrency test to use LinearBinary(linear_window=0) |
test/test_search_anchor_integration.jl |
Removes HintedBinary import and switches to LinearBinary |
test/test_nd_utils_shared.jl |
Updates ND tuple-validation test to remove HintedBinary |
test/test_nd_hint.jl |
Updates comment language to reflect hint-based search without HintedBinary |
test/test_derivatives.jl |
Replaces HintedBinary() with LinearBinary(linear_window=0) in policy iteration |
test/test_coeffs.jl |
Replaces HintedBinary() with LinearBinary(linear_window=0) in keyword-arg coverage |
docs/src/guides/search/policies.md |
Removes HintedBinary policy section and documents linear_window=0 tuning |
docs/src/guides/search/overview.md |
Removes HintedBinary from decision matrix and recommends LinearBinary(linear_window=0) for clustered queries |
docs/src/guides/search/hints.md |
Updates hint documentation and auto-upgrade description post-HintedBinary removal |
docs/src/api/types.md |
Removes HintedBinary from documented API types list |
Comments suppressed due to low confidence (4)
src/core/search.jl:48
- The
Binarydocstring example usesitp(0.5; ...)butitpis never defined in the snippet, so the example as written won’t run. Defineitp = linear_interp(x, y)(or use the one-shotlinear_interp(x, y, ...)) before calling itp.
val = linear_interp(x, y, 0.5; search=Binary()) # explicit binary search
val = linear_interp(x, y, 0.5) # default: AutoSearch()
# With hint: auto-upgrades to LinearBinary() (default window)
hint = Ref(1)
val = itp(0.5; search=Binary(), hint=hint) # uses LinearBinary() internally
docs/src/guides/search/hints.md:45
- The docs describe the Binary+hint auto-upgrade as
LinearBinary{2}. Elsewhere the docs use the user-facing constructor form (LinearBinary()/LinearBinary(linear_window=2)), which is clearer and avoids exposing an internal type parameter in user docs. Consider switching the wording/examples here toLinearBinary()(default window) to stay consistent.
When you provide a `hint` argument with `Binary()`, the search automatically upgrades to `LinearBinary{2}`:
```julia
hint = Ref(1)
val = itp(0.5; search=Binary(), hint=hint) # auto-upgrades to LinearBinary{2}
This also works with the default AutoSearch() — scalar queries resolve to Binary(), and if a hint is provided, they auto-upgrade to LinearBinary{2}:
hint = Ref(1)
val = itp(0.5; hint=hint) # AutoSearch → Binary() → auto-upgrades to LinearBinary{2}**test/test_show.jl:205**
* In this test block the variable name `itp_hinted` is now misleading since it’s created with `LinearBinary(linear_window=0)` (HintedBinary no longer exists). Renaming it (e.g., `itp_lb0`) would make the test intent clearer and avoid confusion when debugging failures.
# Test different search policies
itp_binary = linear_interp(x_vec, y; search=Binary())
itp_hinted = linear_interp(x_vec, y; search=LinearBinary(linear_window=0))
itp_linear_binary = linear_interp(x_vec, y; search=LinearBinary())
itp_linear_binary_16 = linear_interp(x_vec, y; search=LinearBinary(linear_window=16))
**src/core/search.jl:151**
* The “Why Restricted Values?” section above mentions restricting to “powers of 2” and “just 7 variants”, but the allowed list now includes `0` and `128` (9 total values: 0,1,2,4,8,16,32,64,128). Please update that wording/count to match the allowed set (e.g., “0 plus powers of 2” and the correct variant count).
Arguments
linear_window::Integer=2: Size of the linear search window before binary fallback.
Allowed values:0, 1, 2, 4, 8, 16, 32, 64, 128
</details>
---
💡 <a href="/ProjectTorreyPines/FastInterpolations.jl/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
FastInterpolations.jl Benchmarks
Details
| Benchmark suite | Current: 9ccfb5d | Previous: ee1fdff | Ratio |
|---|---|---|---|
10_nd_construct/bicubic_2d |
67216 ns |
63258 ns |
1.06 |
10_nd_construct/bilinear_2d |
2030.4 ns |
1868.7 ns |
1.09 |
10_nd_construct/tricubic_3d |
452995 ns |
434482 ns |
1.04 |
10_nd_construct/trilinear_3d |
4834.82 ns |
4453.32 ns |
1.09 |
11_nd_eval/bicubic_2d_batch |
1802.4 ns |
1836.4 ns |
0.98 |
11_nd_eval/bicubic_2d_scalar |
33.46 ns |
31.86 ns |
1.05 |
11_nd_eval/bilinear_2d_scalar |
28.85 ns |
27.35 ns |
1.05 |
11_nd_eval/tricubic_3d_batch |
3625.8 ns |
3617.8 ns |
1.00 |
11_nd_eval/tricubic_3d_scalar |
56.11 ns |
58.71 ns |
0.96 |
11_nd_eval/trilinear_3d_scalar |
31.35 ns |
33.06 ns |
0.95 |
1_cubic_oneshot/q00001 |
572.06 ns |
592.7 ns |
0.97 |
1_cubic_oneshot/q10000 |
65102.4 ns |
64755.8 ns |
1.01 |
2_cubic_construct/g0100 |
1518.42 ns |
1554.5 ns |
0.98 |
2_cubic_construct/g1000 |
15575.1 ns |
15116.2 ns |
1.03 |
3_cubic_eval/q00001 |
41.48 ns |
39.07 ns |
1.06 |
3_cubic_eval/q00100 |
506.56 ns |
507.96 ns |
1.00 |
3_cubic_eval/q10000 |
45646.1 ns |
45656.3 ns |
1.00 |
4_linear_oneshot/q00001 |
41.78 ns |
42.39 ns |
0.99 |
4_linear_oneshot/q10000 |
37621.2 ns |
37161.4 ns |
1.01 |
5_linear_construct/g0100 |
14.83 ns |
13.92 ns |
1.07 |
5_linear_construct/g1000 |
14.72 ns |
13.13 ns |
1.12 |
6_linear_eval/q00001 |
21.44 ns |
21.94 ns |
0.98 |
6_linear_eval/q00100 |
405.56 ns |
407.56 ns |
1.00 |
6_linear_eval/q10000 |
36673.3 ns |
36437.1 ns |
1.01 |
7_cubic_range/scalar_query |
26.15 ns |
25.95 ns |
1.01 |
7_cubic_vec/scalar_query |
20.14 ns |
17.03 ns |
1.18 |
8_cubic_multi/construct_s001_q100 |
1430.86 ns |
1431.28 ns |
1.00 |
8_cubic_multi/construct_s010_q100 |
6576.22 ns |
6705.52 ns |
0.98 |
8_cubic_multi/construct_s100_q100 |
48898.2 ns |
49816.9 ns |
0.98 |
8_cubic_multi/eval_s001_q100 |
2129.98 ns |
2159.44 ns |
0.99 |
8_cubic_multi/eval_s010_q100 |
4050.16 ns |
3844.38 ns |
1.05 |
8_cubic_multi/eval_s010_q100_scalar_loop |
3658.02 ns |
3656.62 ns |
1.00 |
8_cubic_multi/eval_s100_q100 |
21807.6 ns |
23507.8 ns |
0.93 |
8_cubic_multi/eval_s100_q100_scalar_loop |
6183.5 ns |
6212.6 ns |
1.00 |
9_nd_oneshot/bicubic_2d |
44419.8 ns |
45832.6 ns |
0.97 |
9_nd_oneshot/bilinear_2d |
1722.82 ns |
1706.58 ns |
1.01 |
9_nd_oneshot/tricubic_3d |
382701.8 ns |
382426.1 ns |
1.00 |
9_nd_oneshot/trilinear_3d |
3419.4 ns |
3353.2 ns |
1.02 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'FastInterpolations.jl Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: 9ccfb5d | Previous: ee1fdff | Ratio |
|---|---|---|---|
5_linear_construct/g1000 |
14.72 ns |
13.13 ns |
1.12 |
7_cubic_vec/scalar_query |
20.14 ns |
17.03 ns |
1.18 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 97.81% 97.84% +0.02%
==========================================
Files 71 71
Lines 5731 5712 -19
==========================================
- Hits 5606 5589 -17
+ Misses 125 123 -2
🚀 New features to boost your workflow:
|
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
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.
Summary
Remove
HintedBinarysearch policy entirely.LinearBinary{0}can replace it as a direct equivalent, andBinary()+hintauto-upgrade now routes toLinearBinary()(default window; strictly better default with linear walk).BREAKING CHANGE —
HintedBinaryis no longer exported or available.Motivation
Benchmarks showed
HintedBinaryis dominated byLinearBinaryin every scenario:LinearBinary{2}catches it in 2 steps;HintedBinaryfalls back to full binaryRemoving
HintedBinarysimplifies the search policy surface without losing any capability.Migration
HintedBinary()LinearBinary(linear_window=0)search=HintedBinary()search=LinearBinary(linear_window=0)Searcher{HintedBinary,RefHint}Searcher{LinearBinary{0},RefHint}Binary()+ hint →HintedBinaryBinary()+ hint →LinearBinary()AutoSearch()+ hint →HintedBinaryAutoSearch()+ hint →LinearBinary()Changes
Source (7 files, -79/+23 lines)
HintedBinarystruct,_search_hinted_binary!, all dispatch overloadsLinearBinary(linear_window=0)constructor (was previously invalid)Binary()+hintandAutoSearch()+hint→LinearBinary()(default window)HintedBinaryTests (8 files, -84/+42 lines)
HintedBinaryreferences across test suite_search_hinted_binary!direct coverage testlinear_window=0validity test (now valid, was in invalid list)LinearBinary()(default window)Docs (4 files, -34/+12 lines)
HintedBinarysection from policies guide@docsblockBinary+hint → LinearBinary()(default window)linear_window=0to LinearBinary tuning guideBehavioral Changes
Binary()+ no hintBinaryBinary(unchanged)Binary()+Ref{Int}HintedBinaryLinearBinary()(default window)AutoSearch()+ scalar +Ref{Int}HintedBinaryLinearBinary()(default window)AutoSearch()+ vectorLinearBinary{2}LinearBinary{2}(unchanged)search=HintedBinary()LinearBinary(linear_window=0)Test Results
All targeted test files pass:
test_search,test_show,test_thread_safety,test_derivatives,test_coeffs,test_nd_utils_shared,test_search_anchor_integration.Net: 19 files changed, 77 insertions, 197 deletions — pure simplification.