Inline filtered search with adaptive L#1131
Conversation
…en/two-queue-adaptive-l
…en/two-queue-adaptive-l
…en/two-queue-adaptive-l
…en/two-queue-adaptive-l
hildebrandmw
left a comment
hildebrandmw
left a comment
There was a problem hiding this comment.
Thanks Magdalen! In addition to my inline comments - can you also add some integration tests exercising the functionality here? These go a surprisingly long way towards protecting the algorithm.
Also, can we bikeshed InlineSearch a little? Maybe FilteredSearch? Or InlineFilteredSearch? Not a huge deal, but InlineSearch seems a little opaque to me.
| /// Label evaluator for determining node matches and early termination. | ||
| pub label_evaluator: &'q dyn QueryLabelProvider<InternalId>, | ||
| /// Adaptive L for the search. | ||
| pub adaptive_l: Option<AdaptiveL>, |
There was a problem hiding this comment.
Thoughts on making this a full Algo enum instead of an Option to give us room to expand in the future?
There was a problem hiding this comment.
Not a huge proponent of this idea since hypothetical future variants might want to mix and match with adaptive L rather than plug and play. Seems like we should just leave this to future work if needed.
There was a problem hiding this comment.
There's nothing keeping us from keeping the AdaptiveL struct and using that in the enum like
pub enum Kind {
Standard,
AdaptiveL(AdaptiveL),
}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1131 +/- ##
==========================================
+ Coverage 89.40% 90.49% +1.08%
==========================================
Files 485 487 +2
Lines 92079 92560 +481
==========================================
+ Hits 82323 83759 +1436
+ Misses 9756 8801 -955
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
I've added some integration test. In addition to throwing some of the existing cases in multihop at it, I also designed a test that should return different results depending on whether or not the adaptive L feature is enabled. Regarding the naming, I named it |
| /// specificity = 0.1% (1/1000) → 8× L | ||
| /// and so on up to a pre-set maximum multipler | ||
| #[derive(Debug)] | ||
| pub struct InlineSearch<'q, InternalId> { |
There was a problem hiding this comment.
not a blocker for this PR, but how does this compose with range search and diverse search functionalities?
There was a problem hiding this comment.
From an algorithmic perspective, inline filtering (without any adaptation) will compose seamlessly with any other type of search, because it's just adding the extra step of checking match and keeping track of matched elements. However, from the perspective of our codebase as written right now, it would require new function signatures that accept a LabelProvider.
Thinking about how adaptive L in particular would compose with range search, we could use it within the initial graph search before deciding whether to move on to the unbounded part of range search. Similarly we could compose it with diverse search to increase L_search when few matching candidates are found.
| I: VectorId, | ||
| A: ExpandBeam<T, Id = I> + SearchExt, | ||
| SR: SearchRecord<I> + ?Sized, | ||
| { |
There was a problem hiding this comment.
would we want to allow a paginated API For this function?
There was a problem hiding this comment.
Hmm, that's a good question. Using pagination and adaptive L seems like a strange thing to do. Adapting L_search is morally pretty similar to pagination, since it extends the search for longer when few matched results are found. If there was a user who wanted to use pure inline filtering and control length of the search via pagination only, on the other hand that seems reasonable.
5 participants
No description provided.