Don't store signature in ElasticSearch index #74
Conversation
ecdeveloper
commented
May 9, 2017
ecdeveloper
commented
- Sort/cutoff by elasticsearch relevance score instead (references Signature Compression #2)
|
Hey thanks for submitting. Hopefully I can review this soon. The test is failing because of something on my end, so I have to fix that first. |
|
Hey @ecdeveloper sorry I took so long to get around to this. I fixed the build in master, can you rebase? Thank you |
ecdeveloper
force-pushed
the
no-signature-indexed
branch
3 times, most recently
from
22a0523
to
45afa8c
Compare
|
Thanks, @rhsimplex. I fixed some of my tests, still have to fix a few of them, and probably need to add some tests around checking the score (as you do with dist). |
ecdeveloper
force-pushed
the
no-signature-indexed
branch
2 times, most recently
from
8730577
to
05bdf55
Compare
|
Fixed all tests, except for a single one in |
ecdeveloper
force-pushed
the
no-signature-indexed
branch
from
05bdf55
to
1fda05a
Compare
|
That's the test for adding metadata, which should be ignored in the image search. Maybe the query is including it. I'll look too. |
|
Another update on my side -- since your change affects the behavior on a lot of the code, it's important we make sure the tests are actually hitting all your changes. I'll need to get codecov up and running (which means enlisting the help of @vrde since I no longer work at Ascribe) and possibly update the tests to do all checks with and without signatures. Thanks for your patience, it's just a big change so I want to get it right 🥇 |
|
Sure, no rush here. Thanks for your assistance. |
|
Sorry @ecdeveloper, I haven't forgotten this. Both myself and @vrde were coincidentally on vacation! |
|
@rhsimplex No worries. I just got back from a vacation too ;) |
|
We finally got Codecov. Can you rebase on master so it runs the report? |
* Sort/cutoff by elasticsearch relevance score instead
ecdeveloper
force-pushed
the
no-signature-indexed
branch
from
1fda05a
to
d5d4020
Compare
|
@rhsimplex yay! k, rebased. |
|
Believe it or not, I haven't forgotten about this PR. Looking into why the metadata filter test is failing. |
|
@rhsimplex Come on, man! I believe in you 😄 Actually, no worries. Take your time ;) |
| @@ -117,7 +117,7 @@ def insert_single_record(self, rec): | |||
| raise NotImplementedError | |||
|
|
|||
| def __init__(self, k=16, N=63, n_grid=9, | |||
| crop_percentile=(5, 95), distance_cutoff=0.45, | |||
| crop_percentile=(5, 95), distance_cutoff=0.45, score_cutoff=9.0, | |||
There was a problem hiding this comment.
Stuff for Elasticsearch should be in the Elasticsearch driver. You can include a **kwargs here and pick it up in the child class (tell me if it's not clear what I'm saying).
| @@ -159,6 +159,8 @@ def __init__(self, k=16, N=63, n_grid=9, | |||
| considering how much variance to keep in the image (default (5, 95)) | |||
| distance_cutoff (Optional [float]): maximum image signature distance to | |||
| be considered a match (default 0.45) | |||
There was a problem hiding this comment.
So the default value of 9.0 is the reason the test is failing ... only 8 words match, it looks like. Maybe I need to think a bit more about the default value, but if you change to something less than 8, the tests should pass.
|
|
||
| return formatted_res | ||
|
|
||
| def insert_single_record(self, rec, refresh_after=False): | ||
| rec['timestamp'] = datetime.now() | ||
|
|
||
| # Don't store signature in index | ||
| if 'signature' in rec: |
There was a problem hiding this comment.
We should include a setting in the class constructor whether we want to store the signature or not. The default should be yes, to keep it backwards compatible. I can work on this.
There was a problem hiding this comment.
So you think you can work on that? Or should I handle this?
ps Can't wait to get that PR in ;)
There was a problem hiding this comment.
I can look at it sometime in the near future
