#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)
Change History (20)
comment:1 Changed 7 years ago by
- Owner changed from AlexGhitza to hivert
comment:2 follow-up: ↓ 13 Changed 7 years ago by
- Keywords UniqueRepresentation hash added
- Status changed from new to needs_review
comment:3 follow-up: ↓ 5 Changed 7 years ago by
I'd like to review that patch but I'm missing an example to try.
Paul
comment:4 Changed 7 years ago by
- Status changed from needs_review to needs_work
comment:5 in reply to: ↑ 3 Changed 7 years ago by
- 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: ↓ 7 Changed 7 years ago by
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
- Reviewers set to Paul Zimmerman
- Status changed from needs_work to needs_review
comment:8 Changed 7 years ago by
- 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: ↓ 10 Changed 7 years ago by
- 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
- Status changed from positive_review to needs_work
Replying to nthiery:
Just one thing Florent: please fix the following doctest:
sage: hash(x) # random Trueso that the reader expects some integer result.
Maybe some test like:
sage: type(hash(x)) intcould 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
comment:11 follow-up: ↓ 12 Changed 7 years ago by
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
- 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: ↓ 14 Changed 7 years ago by
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
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
- 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
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
- Milestone set to sage-4.3.3
comment:18 Changed 7 years ago by
- Merged in set to sage-4.3.3.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
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