#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) Commit: 4e068748c51a0ca9ca5c0529bf76fe7309039427
Dependencies: Stopgaps:

Description

part of #24551

Change History (22)

comment:1 Changed 11 months ago by chapoton

  • Branch set to u/chapoton/26088
  • Cc caruso roed added
  • Commit set to 51936350ec6f83cec508bac31fc12ce59510c7b0
  • Status changed from new to needs_review

New commits:

5193635py3: hash for p-adic extension rings

comment:2 Changed 11 months ago by caruso

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 11 months ago by chapoton

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 11 months ago by chapoton

  • Status changed from needs_review to needs_work

some failing doctests..

comment:5 Changed 11 months ago by git

  • Commit changed from 51936350ec6f83cec508bac31fc12ce59510c7b0 to 8c1bde6d3a4d0d02742b898927319233281788a7

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

8c1bde6fix one detail in #26088

comment:6 Changed 11 months ago by chapoton

  • Status changed from needs_work to needs_review

This should fix the failing doctests.

comment:7 Changed 11 months ago by chapoton

green bot, please review

comment:8 Changed 11 months ago by caruso

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 11 months ago by git

  • Commit changed from 8c1bde6d3a4d0d02742b898927319233281788a7 to a7bff82f74ed9b1e40a8029982a6099ba7a8e36d

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

a7bff82better hash #26088

comment:10 follow-up: Changed 11 months ago by chapoton

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 11 months ago by caruso

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: Changed 11 months ago by chapoton

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.

https://docs.python.org/2/library/functions.html#hash

https://docs.python.org/2/glossary.html#term-hashable

comment:13 Changed 11 months ago by chapoton

Is this ok for you, Xavier ? Patchbot is green.

comment:14 in reply to: ↑ 12 Changed 11 months ago by caruso

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 11 months ago by saraedum

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 11 months ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to needs_work

comment:17 Changed 11 months ago by git

  • Commit changed from a7bff82f74ed9b1e40a8029982a6099ba7a8e36d to 4e068748c51a0ca9ca5c0529bf76fe7309039427

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

4e06874do not hash the _printer in #26088

comment:18 Changed 11 months ago by chapoton

  • Status changed from needs_work to needs_review

done

comment:19 Changed 11 months ago by chapoton

Everything seems to be ok. Positive review, somebody ?

comment:20 Changed 11 months ago by chapoton

  • Cc tscrim added

review, please

comment:21 Changed 11 months ago by saraedum

  • Status changed from needs_review to positive_review

comment:22 Changed 11 months ago by vbraun

  • Branch changed from u/chapoton/26088 to 4e068748c51a0ca9ca5c0529bf76fe7309039427
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.