Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8120 closed defect (fixed)

UniqueRepresentation and hash value

Reported by: hivert Owned by:
Priority: major Milestone: sage-4.3.3
Component: algebra Keywords: UniqueRepresentation hash
Cc: zimmerma, nthiery Merged in: sage-4.3.3.alpha0
Authors: Florent Hivert Reviewers: Paul Zimmermann
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (1)

trac_8120-uniquerep_hash-fh.patch (6.6 KB) - added by hivert 7 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by hivert

  • Owner changed from AlexGhitza to hivert

comment:2 follow-up: Changed 7 years ago by hivert

  • Authors set to Florent Hivert
  • Keywords UniqueRepresentation hash added
  • Status changed from new to needs_review

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

comment:3 follow-up: Changed 7 years ago by zimmerma

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

Paul

comment:4 Changed 7 years ago by was

  • Status changed from needs_review to needs_work

comment:5 in reply to: ↑ 3 Changed 7 years ago by hivert

  • Cc zimmerma nthiery added

Hi Paul,

Replying to zimmerma:

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

comment:6 follow-up: Changed 7 years ago by zimmerma

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.

comment:7 in reply to: ↑ 6 Changed 7 years ago by hivert

  • Reviewers set to Paul Zimmerman
  • Status changed from needs_work to needs_review

Replying to zimmerma:

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 ?

comment:8 Changed 7 years ago by zimmerma

  • Status changed from needs_review to positive_review

So are you waiting for another review ?

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

comment:9 follow-up: Changed 7 years ago by nthiery

  • Owner changed from hivert to (none)

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.

comment:10 in reply to: ↑ 9 Changed 7 years ago by hivert

  • Status changed from positive_review to needs_work

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

Changed 7 years ago by hivert

comment:11 follow-up: Changed 7 years ago by zimmerma

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.

comment:12 in reply to: ↑ 11 Changed 7 years ago by hivert

  • Status changed from needs_work to needs_review

Replying to zimmerma:

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" ;-)

comment:13 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by 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 ?

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

comment:14 in reply to: ↑ 13 Changed 7 years ago by nthiery

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.

comment:15 Changed 7 years ago by zimmerma

  • Status changed from needs_review to positive_review

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

comment:16 Changed 7 years ago by mpatel

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

comment:17 Changed 7 years ago by mpatel

  • Milestone set to sage-4.3.3

comment:18 Changed 7 years ago by mpatel

  • Merged in set to sage-4.3.3.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 7 years ago by mpatel

  • Reviewers changed from Paul Zimmerman to Paul Zimmermann

Oops!

Note: See TracTickets for help on using tickets.