Relevancy index with Logistic regression and cosine distance #411
Conversation
|
This pull request introduces 1 alert when merging c8a8f8c into 9697837 - view on LGTM.com new alerts:
|
| @@ -232,6 +232,7 @@ def __init__(self, descriptor_set, idx2uid_kvs, uid2idx_kvs, | |||
| self.random_seed = int(random_seed) | |||
| # Index value for the next added element. Reset to 0 on a build. | |||
| self._next_index = 0 | |||
| self._distance_metric = distance_m | |||
There was a problem hiding this comment.
This doesn't seem to be being used where I would have expected it to be used: in selecting the faiss.METRIC_* value in index construction here (L312). It's still always using L2.
There was a problem hiding this comment.
The intention here is to normalize the vector before we build an index with regular Euclidean distance. By doing so, we build the index with cosine distance.
| @@ -435,6 +436,11 @@ def _build_index(self, descriptors): | |||
|
|
|||
| faiss_index = self._index_factory_wrapper(d, self.factory_string) | |||
| # noinspection PyArgumentList | |||
| if self._distance_metric: | |||
There was a problem hiding this comment.
This seems to be treating the input string value as a boolean. is the intent here to just always normalize descriptors when using FAISS? If so then this behavior should be documented in the class doc-string.
I also see this logic repeated below. I know this isn't a huge about of code, but it might make some sense to break this out into an encapsulated function that both call.
There was a problem hiding this comment.
Made the normalize function a static method.
|
This pull request introduces 2 alerts when merging c719e45 into 9697837 - view on LGTM.com new alerts:
|
Removed unused imports in logistic _reg
The following merge request adds a RelevancyIndex plugin that uses logistic regression and cosine distance for re-ranking IQR results during 'refine.'