Opened 13 months ago
Closed 13 months ago
#32190 closed enhancement (fixed)
Add hashing to class functions
Reported by: | tkarn | Owned by: | tkarn |
---|---|---|---|
Priority: | major | Milestone: | sage-9.4 |
Component: | algebra | Keywords: | gsoc2021 class-function hash |
Cc: | tscrim, tkarn | Merged in: | |
Authors: | Trevor K. Karn | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | cdd59ab (Commits, GitHub, GitLab) | Commit: | cdd59ab74c88fe95d8e19aab6d0495ab74a9050a |
Dependencies: | Stopgaps: |
Description (last modified by )
It is still a to-do item to put ClassFunction
s into the category framework. In the meantime add __hash__
to ClassFunction_gap
so it can be used as an input to a function.
Change History (21)
comment:1 Changed 13 months ago by
- Branch set to u/tkarn/class_function_hash-32190
comment:2 Changed 13 months ago by
- Commit set to 0cdd853d451b14249211b1e3586be4ddcf7bdb35
comment:3 Changed 13 months ago by
- Description modified (diff)
- Owner changed from (none) to tkarn
- Summary changed from Add hash to class functions to Add `CachedRepresentation` to class functions
New commits:
0cdd853 | Add __hash__
|
comment:4 Changed 13 months ago by
- Branch changed from u/tkarn/class_function_hash-32190 to u/tkarn/class_function_cache-32190
- Commit changed from 0cdd853d451b14249211b1e3586be4ddcf7bdb35 to a602e4e05d00a4877b0759d942cda0fe3f597498
- Status changed from new to needs_review
New commits:
a602e4e | Add CachedRepresentation to ClassFunction_gap
|
comment:5 Changed 13 months ago by
I don't see why making this a CachedRepresentation
is needed to do that. All you would need is the equality comparison and a __hash__
. Can you explain the reasoning in a bit more detail?
comment:6 Changed 13 months ago by
My main reasoning was that I wanted to pass an instance of ClassFunction
as an argument of FiniteDimensionalTwistedInvariantModule.__init__
of #32145 and when doing doctesting I was getting errors which seemed to be because of cacheing (FiniteDimensionalTwistedInvariantModule
inherits from UniqueRepresentation
via SubmoduleWithBasis
). It did work to fix it with a hash (c.f. commit 0cdd853 on branch u/tkarn/class_function_hash-32190) but I didn't see anywhere else in source code that directly implemented a __hash__
and so I thought CachedRepresentation
seemed like the right way to do it following the general Sage conventions.
comment:7 Changed 13 months ago by
Thanks for your frank reply. See documentation. The classes CachedRepresentation
and UniqueRepresentation
are to establish nonlocal objects. They introduce extra overhead, with possible savings when objects get created with equivalent construction parameters repeatedly, but they do that at the cost of modified semantics (you should really not change anything on these objects, because your changes may affect unrelated code). Oddly enough, CachedRepresentation
does *not* define equality or hashing behaviour, so it shouldn't even do what you're using it for.
comment:8 follow-up: ↓ 10 Changed 13 months ago by
Thank you for the explanation. That makes sense. I understand why hashing behavior is needed for my desired use-case, but could you explain why equality behavior would be needed?
comment:9 Changed 13 months ago by
- Branch changed from u/tkarn/class_function_cache-32190 to u/tkarn/class_function_hash-32190
- Commit changed from a602e4e05d00a4877b0759d942cda0fe3f597498 to 0cdd853d451b14249211b1e3586be4ddcf7bdb35
- Description modified (diff)
- Status changed from needs_review to needs_work
- Summary changed from Add `CachedRepresentation` to class functions to Add hashing to class functions
New commits:
0cdd853 | Add __hash__
|
comment:10 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 13 months ago by
Replying to tkarn:
Thank you for the explanation. That makes sense. I understand why hashing behavior is needed for my desired use-case, but could you explain why equality behavior would be needed?
That's just a python thing: hashable objects must be comparable for equality, and items that compare equal should have identical hashes. Without that, dictionary lookup and sets don't work properly (and hashable objects must play nice with sets and dictionaries -- that's what hashing is for). Because hash and equality need to be coordinated, it's normally not even possible in python to inherit one but not the other.
(sage actually violates the python hash contract, because 1 == GF(5)(1) == 6
. This causes trouble, but it is required to have ==
work properly with coercion, which was felt important for a CAS)
comment:11 in reply to: ↑ 10 ; follow-up: ↓ 14 Changed 13 months ago by
That makes sense thanks. ClassFunction_gap
already has @richcmp_method
so when I try to implement __eq__
, I get the error
TypeError: class <class 'sage.groups.class_function.ClassFunction_gap'> defines __eq__ which cannot be combined with @richcmp_method
Does that mean that I don't need to implement an __eq__
and it suffices to implement just a hash?
comment:12 Changed 13 months ago by
- Commit changed from 0cdd853d451b14249211b1e3586be4ddcf7bdb35 to d2f701149d765e18eb37fc5956dd3058b610be22
Branch pushed to git repo; I updated commit sha1. New commits:
d2f7011 | Add test for __hash__
|
comment:13 Changed 13 months ago by
- Status changed from needs_work to needs_review
comment:14 in reply to: ↑ 11 Changed 13 months ago by
Replying to tkarn:
That makes sense thanks.
ClassFunction_gap
already has@richcmp_method
so when I try to implement__eq__
, I get the errorTypeError: class <class 'sage.groups.class_function.ClassFunction_gap'> defines __eq__ which cannot be combined with @richcmp_methodDoes that mean that I don't need to implement an
__eq__
and it suffices to implement just a hash?
Yes, that's correct. The _richcmp_
covers all comparison operators, in particular ==
.
I would propose a slightly different hash function:
def __hash__(self): return hash((self._group, self.values())
These are the two things that are checked for equality, but you get distinct hashes for all of the class functions. Right now, all class functions have the same repr, so they all have the same hash (which is less than ideal).
It might also be good to put the __hash__
method below the __richcmp__
as typically the __init__
is the first method.
comment:15 Changed 13 months ago by
- Status changed from needs_review to needs_work
comment:16 Changed 13 months ago by
- Commit changed from d2f701149d765e18eb37fc5956dd3058b610be22 to 6834c91fbe9d19b8ec3a13e377a64a85d94db7d4
Branch pushed to git repo; I updated commit sha1. New commits:
6834c91 | Base __hash__ off of self._group, self.values()
|
comment:17 Changed 13 months ago by
- Status changed from needs_work to needs_review
comment:18 Changed 13 months ago by
- Description modified (diff)
comment:19 Changed 13 months ago by
- Commit changed from 6834c91fbe9d19b8ec3a13e377a64a85d94db7d4 to cdd59ab74c88fe95d8e19aab6d0495ab74a9050a
Branch pushed to git repo; I updated commit sha1. New commits:
cdd59ab | Remove .values()
|
comment:20 Changed 13 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM. Thank you.
comment:21 Changed 13 months ago by
- Branch changed from u/tkarn/class_function_hash-32190 to cdd59ab74c88fe95d8e19aab6d0495ab74a9050a
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Add __hash__