Skip to content
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

ImageMultiHash __sub__ is not commutative #152

Open
nh2 opened this issue Nov 25, 2021 · 3 comments
Open

ImageMultiHash __sub__ is not commutative #152

nh2 opened this issue Nov 25, 2021 · 3 comments

Comments

@nh2
Copy link

nh2 commented Nov 25, 2021

``subonImageMultiHash` on 2 different images is in most cases not commutative.

For example,

a - b = 4.25
b - a = 10.25

hash_diff() is communative. It is very confusing to me that from a commutative result (the (matches, sum_distance) tuple), a non-commutative distance metric is derived.

This creates nonsensical results when trying to find the most similar match for every image in a set of images (e.g. using best_match()), because the sorting by metric now depends on the order in which the images are given.

It seems to me that best_match() would be better implemented by simply sorting the (matches, sum_distance) results descending by matches, ascending by sum_distance, instead of going via this non-commutative __sub__.

Do you agree?

(Also, __sub__ on ImageHash is commutative.)

@JohannesBuchner
Copy link
Owner

pinging @joshcoales who contributed this code and perhaps has an opinion.

@SpangleLabs
Copy link

Apologies, yeah, I remember this being an absolute pain.

I agree that sorting matches descending, then sum_distance ascending would be best. I was trying to merge that down into a single int for comparison, but it's a bit of a nightmare, yeah.

@JohannesBuchner
Copy link
Owner

Perhaps you could prepare a PR (plus test of commutativity, perhaps for all hash types), and add comments with some demonstrations of the behaviour before and after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants