UniqueRepresentation and hash value

[hivert]

The documentation of UniqueRepresentation says

    Similarly, the identity is used as hash function, which is also as
    fast as it can get. However this means that the hash function may
    change in between Sage sessions, or even within the same Sage
    session (see below). Subclasses should overload :meth:`__hash__`
    if this could be a problem.

But there is no implementation of __hash__.

I'm adding one.

CC: @zimmermann6 @nthiery

Component: algebra

Keywords: UniqueRepresentation hash

Author: Florent Hivert

Reviewer: Paul Zimmermann

Merged: sage-4.3.3.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/8120

[hivert]

Changed keywords from none to UniqueRepresentation hash

[hivert]

comment:2

I just submitted a patch which comply with the documentation. However, since we are in UniqueRepresentations we could use __classcall__ which knows the parameters that constructed the object to insert into the created object the hash value of those parameters in the same veins it insert some stuff needed for pickling/unpickling. Any comment about that ?

Florent

[hivert]

Author: Florent Hivert

[zimmermann6]

comment:3

I'd like to review that patch but I'm missing an example to try.

Paul

[hivert]

comment:5

Hi Paul,

Replying to @zimmermann6:

I'd like to review that patch but I'm missing an example to try.

I'm not sure what you want: in the patch you have the following tests example:

sage: class bla(UniqueRepresentation, SageObject): pass 
sage: x = bla(); hx = hash(x) 
sage: x.rename("toto")    
sage: hx == hash(x) 
True 

If you want more elaborated examples using UniqueRepresentation, they are dozens of them through sage library (the ultimate goal is that nearly each parent inherits from this). Pick you favorite one (prime.py is a good example see #7397):

tomahawk-*ge-combinat/sage $ grep -l UniqueRepresentation **/*.py
categories/category.py
categories/enumerated_sets.py
categories/examples/commutative_additive_monoids.py
categories/examples/commutative_additive_semigroups.py
categories/examples/finite_coxeter_groups.py
categories/examples/finite_enumerated_sets.py
categories/examples/finite_monoids.py
categories/examples/finite_semigroups.py
categories/examples/finite_weyl_groups.py
categories/examples/infinite_enumerated_sets.py
categories/examples/monoids.py
categories/examples/semigroups.py
categories/examples/sets_cat.py
categories/primer.py
categories/semigroups.py
categories/sets_cat.py
combinat/crystals/affine.py
combinat/crystals/letters.py
combinat/free_module.py
combinat/root_system/cartan_type.py
combinat/root_system/root_system.py
combinat/root_system/type_dual.py
combinat/root_system/type_relabel.py
combinat/root_system/weyl_group.py
combinat/sf/sf.py
groups/matrix_gps/general_linear.py
groups/matrix_gps/special_linear.py
groups/perm_gps/permgroup_named.py
misc/classcall_metaclass.py
misc/constant_function.py
misc/nested_class_test.py
sets/disjoint_union_enumerated_sets.py
sets/finite_enumerated_set.py
sets/non_negative_integers.py
sets/primes.py
structure/all.py
structure/dynamic_class.py
structure/unique_representation.py

[zimmermann6]

comment:6

I tried the patch and sage -t * and got the same failures that I get without the patch
(on x86_64 Fedora12, see #7773). Thus a positive review for me.

[hivert]

Reviewer: Paul Zimmerman

[hivert]

comment:7

Replying to @zimmermann6:

I tried the patch and sage -t * and got the same failures that I get without the patch
(on x86_64 Fedora12, see #7773). Thus a positive review for me.

So are you waiting for another review ? Or did you simply forgot to check the relevant box ?

[zimmermann6]

comment:8

So are you waiting for another review ?

no, I was waiting for the "needs review" status.

[nthiery]

comment:9

Just one thing Florent: please fix the following doctest:

sage: hash(x) # random 
True

so that the reader expects some integer result.

Maybe some test like:

sage: type(hash(x))
int

could be added.

[hivert]

comment:10

Replying to @nthiery:

Just one thing Florent: please fix the following doctest:

sage: hash(x) # random 
True

so that the reader expects some integer result.

Maybe some test like:

sage: type(hash(x))
int

could be added.

Oups !!! I'm resubmitting a patch with the following diff:

diff --git a/sage/structure/unique_representation.py b/sage/structure/unique_represent
ation.py
--- a/sage/structure/unique_representation.py
+++ b/sage/structure/unique_representation.py
@@ -483,7 +483,9 @@ class UniqueRepresentation:
             sage: x = UniqueRepresentation()
             sage: y = UniqueRepresentation()
             sage: hash(x) # random
-            True
+            74153040
+            sage: type(hash(x))
+            <type 'int'>
             sage: hash(x) == hash(y)
             True
             sage: class bla(UniqueRepresentation, SageObject): pass

Please re-review...

Florent

[hivert]

Attachment: trac_8120-uniquerep_hash-fh.patch.gz

[zimmermann6]

comment:11

Please re-review...

will do it as soon as my current sage -t * finishes. In the meantime you can click on
"needs review", unless more work is needed.

[hivert]

comment:12

Replying to @zimmermann6:

Please re-review...

will do it as soon as my current sage -t * finishes. In the meantime you can click on
"needs review", unless more work is needed.

Actually, the precise button I needed to hit was "Submit Changes" ;-)

[hivert]

comment:13

Nicolas (and also Paul), you didn't comment about the following thought:

However, since we are in UniqueRepresentations we could use __classcall__ which knows the parameters that constructed the object to insert into the created object the hash value of those parameters in the same veins it insert some stuff needed for pickling/unpickling. Any comment about that ?

Anyway, whatever is decided, I suggest to keep it for a forthcomming ticket, since it can raise some backward compatibility issues...

[nthiery]

comment:14

Replying to @hivert:

Nicolas (and also Paul), you didn't comment about the following thought:

However, since we are in UniqueRepresentations we could use __classcall__ which knows the parameters that constructed the object to insert into the created object the hash value of those parameters in the same veins it insert some stuff needed for pickling/unpickling. Any comment about that ?

That sounds good. We should probably include the class of the object in the hash calculation.

Anyway, whatever is decided, I suggest to keep it for a forthcomming ticket, since it can raise some backward compatibility issues...

Yup.

[zimmermann6]

comment:15

I did try with the new patch, and it is ok. Thus a positive review.

[qed777]

comment:16

The ticket number is missing from the commit string. I've refreshed the patch I've applied to 4.3.3.alpha0.

[qed777]

Merged: sage-4.3.3.alpha0

[qed777]

comment:19

Oops!

[qed777]

Changed reviewer from Paul Zimmerman to Paul Zimmermann