Opened 2 years ago

Last modified 10 days ago

#30498 new defect

Hash function of libgap objects

Reported by: vdelecroix Owned by:
Priority: major Milestone:
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description (last modified by dimpase)

Libgap object hash function is not compatible with Python/Sage

sage: hash(2)
2
sage: hash(libgap(2))
4749963527729243378

As a consequence

sage: set([libgap(2)]) == set([2])
False

Note that the implementation of __hash__ is not very subtle hash(str(self)) and could easily be modified to handle properly integers and rationals.

Change History (18)

comment:1 Changed 2 years ago by vdelecroix

Description: modified (diff)

comment:2 Changed 2 years ago by dimpase

Description: modified (diff)

comment:3 Changed 2 years ago by dimpase

Description: modified (diff)

comment:4 Changed 2 years ago by dimpase

Is libgap an exception?

sage: hash(singular(2))                                                                                                                                                                                                                
-3887916961774677074
sage: hash(pari(2))                                                                                                                                                                                                                    
-5620492333743569407
sage: hash(maxima_calculus(2))                                                                                                                                                                                                         
-7166118788106343663

comment:5 in reply to:  4 Changed 2 years ago by vdelecroix

Replying to dimpase:

Is libgap an exception?

sage: hash(singular(2))                                                                                                                                                                                                                
-3887916961774677074
sage: hash(pari(2))                                                                                                                                                                                                                    
-5620492333743569407
sage: hash(maxima_calculus(2))                                                                                                                                                                                                         
-7166118788106343663

Good point! But I would prefer to fix the various interfaces with various tickets. In particular for pari, the hash comes from cypari2 and is implemented using a C function from the PARI libaray. It is not on Sage side. The specificity of the libgap interface is that it is using hash(str(self)) which can easily adapted to match Sage on integers/rationals. Feel free to open further tickets for singular and maxima.

comment:6 Changed 2 years ago by dimpase

I am not convinced that this is a good idea. Why would a hash of differently typed things be equal? To me, the fact that ZZ(2) and int(2) both hash to 2 is more of a hassle, i.e., a hash collision! But I might be wrong - should we discuss this on sage-devel?

comment:7 Changed 2 years ago by vdelecroix

https://docs.python.org/3/reference/datamodel.html#object.__hash

[...] The only required property is that objects which compare
equal have the same hash value [...]
Last edited 2 years ago by vdelecroix (previous) (diff)

comment:8 Changed 2 years ago by dimpase

I am not sure whether this only applies to objects of the same class/type.

comment:9 in reply to:  8 ; Changed 2 years ago by nbruin

Replying to dimpase:

I am not sure whether this only applies to objects of the same class/type.

The python spec has no such requirement, which means that generally, objects for different class/type should compare unequal.

In sage, our equality is too liberal to combine useful hashes with keeping to this strict rule:

sage: 2 == GF(3)(2) == 5
True

so we're already forced to be pragmatic about it. Within the same parent, I think we do want it to hold -- otherwise you're better off making the objects not hashable. Outside of that, we can do a reasonable effort, but things will break eventually. People will have to learn that in Sage, you need to normalize your keys prior to putting them in a dictionary, and what that means depends on your application. It will generally mean forcing all the keys into the same parent. It's important to keep hashes cheap too!

comment:10 in reply to:  9 Changed 2 years ago by vdelecroix

Replying to nbruin:

It's important to keep hashes cheap too!

Regarding that point, hash(str(self)) is slow!

comment:11 Changed 2 years ago by mkoeppe

Milestone: sage-9.2sage-9.3

comment:12 Changed 2 years ago by mkoeppe

Milestone: sage-9.3sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:13 Changed 2 years ago by embray

I opened an issue for this against gappy as well: https://github.com/embray/gappy/issues/11

comment:14 Changed 19 months ago by mkoeppe

Milestone: sage-9.4sage-9.5

comment:15 Changed 14 months ago by mkoeppe

Milestone: sage-9.5sage-9.6

comment:16 Changed 9 months ago by mkoeppe

Milestone: sage-9.6sage-9.7

comment:17 Changed 5 months ago by mkoeppe

Milestone: sage-9.7sage-9.8

comment:18 Changed 10 days ago by mkoeppe

Milestone: sage-9.8
Note: See TracTickets for help on using tickets.