Update benchmarks with PDQ Faiss results #1755
Conversation
| faiss_threads : 1 | ||
| dataset_size : 10000 | ||
| num_queries : 1000 | ||
| thresholds : [255] |
There was a problem hiding this comment.
This is a very unrealistic threshold! We should probably only be testing at 31 - this is why your multihash results are so poor, since you are forcing it search every subtree instead of the ones nearby.
There was a problem hiding this comment.
Lol yep; totally right there. I thought a high threshold enforces strict matching. Changed to 31
| @@ -1,45 +1,81 @@ | |||
| # pytx-vpdq | |||
| Benchmark vPDQ implementation in threatexchange library | |||
| # pytx-vPDQ | |||
There was a problem hiding this comment.
blocking q: I am confused by why this is here instead of python-threatexchange/vpdq/README.md
Any ideas?
There was a problem hiding this comment.
Not sure either... maybe not to clutter the docs? I can move it -- up to you
Don't think there was a specific reasoning originally here either tho
There was a problem hiding this comment.
I think it probably makes more sense in vpdq, if you want to move it in a followup will leave for you.
| else: | ||
| raise ValueError("Invalid test type") |
There was a problem hiding this comment.
This seems useful to keep in, since the error will otherwise be on L76 with variable undefined. This is else is also to defend against future developers adding a new test_type and forgetting to update it here.
There was a problem hiding this comment.
Kk; I thought argparse would stop before we reached here, so that's why I did it
Reverted it back tho
There was a problem hiding this comment.
It would in normal operation - this else is to defend against a future developer adding it to the argparse and then forgetting to add a case here. This error is meant to save them time debugging.
| @@ -1,45 +1,81 @@ | |||
| # pytx-vpdq | |||
| Benchmark vPDQ implementation in threatexchange library | |||
| # pytx-vPDQ | |||
There was a problem hiding this comment.
I think it probably makes more sense in vpdq, if you want to move it in a followup will leave for you.
| PDQFlatHashIndex - Total Time to search (s): 0.012083053588867188 | ||
| PDQMultiHashIndex - Total Time to search (s): 0.01529383659362793 |
There was a problem hiding this comment.
Hmm, this is closer to what I would expect, but this might be worth digging more into later. I think we are choosing the wrong thresholds for these.
Summary
Update the benchmarks, plus
benchmark_pdq_faiss_matchers.pydidn't have any results in the markdown file, so I added that as well.Test Plan
Yes