-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add distance threshold optimizer classes #292
base: main
Are you sure you want to change the base?
Conversation
rbs333
commented
Feb 27, 2025
- add base, router, and cache threshold optimizer
- uses ranx to support eval metrics
- class is designed to receive a optimizer class in the case where someone wants to supply their own without us having to make a release to support something custom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Left some of my feedback but I am pumped about this.
PRECISION = "precision" | ||
RECALL = "recall" | ||
|
||
def __str__(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think EvalMetric(str, Enum)
would do the trick? See how we handle other enums in the lib
return max(metrics.items(), key=lambda x: (x[1]["score"], -x[0]))[0] | ||
|
||
|
||
def _grid_search_opt_cache( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason these aren't members of the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought process is this shows that you can pass in any function that meets this definition not dependant on any class state if you wanted to go a more custom route. It's a little atypical though since it is still an internal method that doesn't really make sense to expose outside of this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add imports here similar to vectorizers so we can do from redisvl.utils.threshold_optimizer import CacheThresholdOptimizer
Might also consider just naming it redisvl.utils.optimize
to follow the same grammar as redisvl.utils.vectorize