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:

Status badges

Description (last modified by tkarn)

It is still a to-do item to put ClassFunctions 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 tkarn

  • Branch set to u/tkarn/class_function_hash-32190

comment:2 Changed 13 months ago by git

  • Commit set to 0cdd853d451b14249211b1e3586be4ddcf7bdb35

Branch pushed to git repo; I updated commit sha1. New commits:

​0cdd853Add __hash__

comment:3 Changed 13 months ago by tkarn

  • 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:

​0cdd853Add __hash__

comment:4 Changed 13 months ago by tkarn

  • 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:

​a602e4eAdd CachedRepresentation to ClassFunction_gap

comment:5 Changed 13 months ago by tscrim

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 tkarn

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 nbruin

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: Changed 13 months ago by 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?

comment:9 Changed 13 months ago by tkarn

  • 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:

​0cdd853Add __hash__

comment:10 in reply to: ↑ 8 ; follow-up: Changed 13 months ago by nbruin

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: Changed 13 months ago by tkarn

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 git

  • Commit changed from 0cdd853d451b14249211b1e3586be4ddcf7bdb35 to d2f701149d765e18eb37fc5956dd3058b610be22

Branch pushed to git repo; I updated commit sha1. New commits:

​d2f7011Add test for __hash__

comment:13 Changed 13 months ago by tkarn

  • Status changed from needs_work to needs_review

comment:14 in reply to: ↑ 11 Changed 13 months ago by tscrim

Replying to tkarn:

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?

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 tkarn

  • Status changed from needs_review to needs_work

comment:16 Changed 13 months ago by git

  • Commit changed from d2f701149d765e18eb37fc5956dd3058b610be22 to 6834c91fbe9d19b8ec3a13e377a64a85d94db7d4

Branch pushed to git repo; I updated commit sha1. New commits:

​6834c91Base __hash__ off of self._group, self.values()

comment:17 Changed 13 months ago by tkarn

  • Status changed from needs_work to needs_review

comment:18 Changed 13 months ago by tkarn

  • Description modified (diff)

comment:19 Changed 13 months ago by git

  • Commit changed from 6834c91fbe9d19b8ec3a13e377a64a85d94db7d4 to cdd59ab74c88fe95d8e19aab6d0495ab74a9050a

Branch pushed to git repo; I updated commit sha1. New commits:

​cdd59abRemove .values()

comment:20 Changed 13 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM. Thank you.

comment:21 Changed 13 months ago by vbraun

  • Branch changed from u/tkarn/class_function_hash-32190 to cdd59ab74c88fe95d8e19aab6d0495ab74a9050a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.