rhsimplex / image-match

🎇 Quickly search over billions of images
2.94k stars 405 forks source link

Don't store signature in ElasticSearch index #74

Open ecdeveloper opened 7 years ago

ecdeveloper commented 7 years ago
rhsimplex commented 7 years ago

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.

rhsimplex commented 7 years ago

Hey @ecdeveloper sorry I took so long to get around to this. I fixed the build in master, can you rebase?

Thank you

ecdeveloper commented 7 years ago

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 commented 7 years ago

Fixed all tests, except for a single one in test_elasticsearch_driver_metadata_as_nested. Will take a closer look tomorrow.

rhsimplex commented 7 years ago

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.

rhsimplex commented 7 years ago

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 :1st_place_medal:

ecdeveloper commented 7 years ago

Sure, no rush here. Thanks for your assistance.

rhsimplex commented 7 years ago

Sorry @ecdeveloper, I haven't forgotten this. Both myself and @vrde were coincidentally on vacation!

ecdeveloper commented 7 years ago

@rhsimplex No worries. I just got back from a vacation too ;)

rhsimplex commented 7 years ago

We finally got Codecov. Can you rebase on master so it runs the report?

ecdeveloper commented 7 years ago

@rhsimplex yay! k, rebased.

rhsimplex commented 7 years ago

Believe it or not, I haven't forgotten about this PR. Looking into why the metadata filter test is failing.

ecdeveloper commented 7 years ago

@rhsimplex Come on, man! I believe in you 😄 Actually, no worries. Take your time ;)