Opened 2 years ago
Closed 2 years ago
#26088 closed enhancement (fixed)
py3: hash for padic extension rings
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  python3  Keywords:  
Cc:  caruso, roed, tscrim  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Julian Rüth 
Report Upstream:  N/A  Work issues:  
Branch:  4e06874 (Commits)  Commit:  4e068748c51a0ca9ca5c0529bf76fe7309039427 
Dependencies:  Stopgaps: 
Description
part of #24551
Change History (22)
comment:1 Changed 2 years ago by
 Branch set to u/chapoton/26088
 Cc caruso roed added
 Commit set to 51936350ec6f83cec508bac31fc12ce59510c7b0
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
I'm not sure what hashes are used for but I would say that the precision type should be included in the hash, shouldn't it?
comment:3 Changed 2 years ago by
This hash is parallel to the behaviour of __eq__
in the same class.
I you want to change the equality, maybe do that in another ticket ?
comment:4 Changed 2 years ago by
 Status changed from needs_review to needs_work
some failing doctests..
comment:5 Changed 2 years ago by
 Commit changed from 51936350ec6f83cec508bac31fc12ce59510c7b0 to 8c1bde6d3a4d0d02742b898927319233281788a7
Branch pushed to git repo; I updated commit sha1. New commits:
8c1bde6  fix one detail in #26088

comment:6 Changed 2 years ago by
 Status changed from needs_work to needs_review
This should fix the failing doctests.
comment:7 Changed 2 years ago by
green bot, please review
comment:8 Changed 2 years ago by
I think that it's not enough to compare the string representation of the printer. For example:
sage: A.<pi> = Zp(2, print_mode='bars', print_sep='').extension(x^3  2) sage: B.<pi> = Zp(2, print_mode='bars', print_sep='#').extension(x^3  2) sage: A == B False sage: hash(A) == hash(B) True
Probably, one should implement a method __hash__
for the printer as well...
comment:9 Changed 2 years ago by
 Commit changed from 8c1bde6d3a4d0d02742b898927319233281788a7 to a7bff82f74ed9b1e40a8029982a6099ba7a8e36d
Branch pushed to git repo; I updated commit sha1. New commits:
a7bff82  better hash #26088

comment:10 followup: ↓ 11 Changed 2 years ago by
The hash is not necessarily taking all parameters into account. One could very well just forget about the printing mode.
Nevertheless, here is another proposal.
comment:11 in reply to: ↑ 10 Changed 2 years ago by
Replying to chapoton:
The hash is not necessarily taking all parameters into account. One could very well just forget about the printing mode.
What's exactly the purpose and the requirements on the hash?
Do we need to ensure that A == B
implies hash(A) == hash(B)
? Or the other way around? Something else? Nothing?
comment:12 followup: ↓ 14 Changed 2 years ago by
The only requirement is that "A == B" implies "hash(A) == hash(B)".
One could even use the constant hash, that would be perfectly valid, but useless. So in general there is a balance or a choice : complex hash that tells many things apart, or simple hash that is not so good.
I think in sage, the hash for parents are not so important. They are required for python3 nevertheless.
comment:13 Changed 2 years ago by
Is this ok for you, Xavier ? Patchbot is green.
comment:14 in reply to: ↑ 12 Changed 2 years ago by
Yes, it looks good to me.
I would be happy to give a positive review but I don't know so much about padic printers and I would be more comfortable if David confirms that your changes won't break something elsewhere. David, could you do this, please?
comment:15 Changed 2 years ago by
A printer's __eq__
is quite complex so I think that hashing the full dict is too much, i.e., it'll give different hashes to printers that are considered equal. Strangely, the printer is not hashable (seems like a bug to me) so I'd not include it into the hash of the ring at all. If it were hashable, we should just include the printer's hash value.
So, I'd drop hash_printer
and add a comment that printer's are strangely not hashable. Does that sound Ok to you?
comment:16 Changed 2 years ago by
 Reviewers set to Julian Rüth
 Status changed from needs_review to needs_work
comment:17 Changed 2 years ago by
 Commit changed from a7bff82f74ed9b1e40a8029982a6099ba7a8e36d to 4e068748c51a0ca9ca5c0529bf76fe7309039427
Branch pushed to git repo; I updated commit sha1. New commits:
4e06874  do not hash the _printer in #26088

comment:19 Changed 2 years ago by
Everything seems to be ok. Positive review, somebody ?
comment:21 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:22 Changed 2 years ago by
 Branch changed from u/chapoton/26088 to 4e068748c51a0ca9ca5c0529bf76fe7309039427
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: hash for padic extension rings