Opened 4 years ago
Closed 4 years ago
#26088 closed enhancement (fixed)
py3: hash for padic extension rings
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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, GitHub, GitLab) | Commit: | 4e068748c51a0ca9ca5c0529bf76fe7309039427 |
Dependencies: | Stopgaps: |
Description
part of #24551
Change History (22)
comment:1 Changed 4 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 4 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 4 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 4 years ago by
- Status changed from needs_review to needs_work
some failing doctests..
comment:5 Changed 4 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 4 years ago by
- Status changed from needs_work to needs_review
This should fix the failing doctests.
comment:7 Changed 4 years ago by
green bot, please review
comment:8 Changed 4 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 4 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 follow-up: ↓ 11 Changed 4 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 4 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 follow-up: ↓ 14 Changed 4 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 4 years ago by
Is this ok for you, Xavier ? Patchbot is green.
comment:14 in reply to: ↑ 12 Changed 4 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 p-adic 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 4 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 4 years ago by
- Reviewers set to Julian Rüth
- Status changed from needs_review to needs_work
comment:17 Changed 4 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 4 years ago by
Everything seems to be ok. Positive review, somebody ?
comment:21 Changed 4 years ago by
- Status changed from needs_review to positive_review
comment:22 Changed 4 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 p-adic extension rings