hash functions seem to be inconsistent with object equality

[thisiscam]

Describe the bug
Hash function does not respect object equality.

To Reproduce
Steps to reproduce the behavior:

import islpy
a = islpy.MultiAff("{ [i] -> [i] }")
b = islpy.MultiAff("{ [j] -> [j] }")
print(a == b) # prints "True"
print(hash(a) == hash(b)) # prints "False"

Expected behavior
I expect the hash function to be equal when the objects are equal.

Environment (please complete the following information):

• OS: MacOS
• Python version: 3.8

My Observations
It seems like the islpy wrapper is requires a <obj>_get_hash function in ISL to work properly, or else will fall back to a hash of the string: https://github.com/inducer/islpy/blob/main/islpy/__init__.py#L323. I believe those get_hash functions do not go through the wrapper script for some reason.

[inducer]

Thanks for the report! I agree that the string fallback for the hash is a bad idea, because it's stricter than isl's notion of equality. (Personally, I find it weird that identifiers are immaterial to object equality, but that's isl's policy decision to make, not islpy's.) #90 is a test balloon to see how much breaks if we get rid of the fallback.

[thisiscam]

I just realized that ISL's get_hash family of functions are just not public for some reason (which is why they didn't propagate to islpy).

I guess for now they just have to be considered as not hashable.

[inducer]

I just realized that ISL's get_hash family of functions are just not public for some reason (which is why they didn't propagate to islpy).

Islpy doesn't give a hoot about whether isl marks a function public, it just exposes it. I guess that's something that I should mention in the README, and perhaps also note in the documentation about each function.

[thisiscam]

Islpy doesn't give a hoot about whether isl marks a function public, it just exposes it.

Could you clarify that? My memory was that islpy does not read those isl_<type>_private.h header files? For example, the hash function for Map appears to be at: https://github.com/inducer/isl/blob/3ea3e8b77475bea49b35b8215d8f66af94121b3c/isl_map_private.h#L263

[inducer]

Could you clarify that?

For sure! There are the *_private.h headers in the isl source, and it is true that islpy does not consider them. In the (notionally) public headers under include/isl/, some functions are marked __isl_export and some are not. islpy exposes all functions in those headers that it understands, irrespective of the export mark. As of #91, warnings appear in the docs about those functions. If it's important to you, one could also warn about their use at runtime, though this would have to be a (run-time) knob.

[inducer]

isl_map_get_hash appears in the public header:

include/isl/map.h:692:uint32_t isl_map_get_hash(__isl_keep isl_map *map);

[thisiscam]

Oops, my bad -- didn't find that one!

Then this is interesting, since

islpy.Map.get_hash

gives me:

AttributeError: type object 'islpy._isl.Map' has no attribute 'get_hash'

Perhaps get_hash failed to parse by gen_wrap.py?

[inducer]

Perhaps get_hash failed to parse by gen_wrap.py?

Nope, it all goes according to plan. get_hash gets exposed as __hash__ right away. The attribute renaming stuff was never necessary and should have been deleted long ago:

islpy/gen_wrap.py

Lines 1388 to 1389 in ffd8df3

	if meth.name == "get_hash" and len(meth.args) == 1:
	py_name = "__hash__"

[thisiscam]

Thanks! So basically ISL's hash function does not respect equality?

I wonder if we can fix the hash: Perhaps we can hash the space signature plus the constraints matrix (with some normalization procedure if necessary). But I guess this is better implemented in ISL than islpy.

[inducer]

Thanks! So basically ISL's hash function does not respect equality?

I have no evidence to suggest that they do not. AFAICT, the issue was that the logic you pointed to unnecessarily overwrote the perfectly good hash supplied by ISL with a string-based fallback that had issues, as you pointed out.