py3: adding hash function for orders and fraction fields

[chapoton]

+ a few other details

[chapoton]

New commits:

​a0f9b86

py3: adding hash to congruence groups and fraction fields + other details

[chapoton]

some failing doctests

[git]

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

​ac15631

trac 25586 fixing new hash of congruence groups

[vdelecroix]

Similar to #25591: the hash of a fraction field should be different from the hash of its underlying ring.

What you did for congruence subgroups is not a very good idea as it will break equality

sage: G = Gamma(2)
sage: H = G.as_permutation_group()
sage: G == H
True

One (expensive) possibility for now would be

def __hash__(self):
    return hash(self.as_permutation_group())

[git]

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

​a56f973

fixing hash of fraction fields and congruence groups

[chapoton]

Merci. C'est fait aussi.

[chapoton]

ping ?

[vdelecroix]

désolé... d'autres sollicitations.

[chapoton]

Je comprends. Merci beaucoup.

[vbraun]

See patchbot

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​97d8e09

py3: adding hash to orders and fraction fields + other details

[chapoton]

the case of congruence groups being harder, it is no longer handled here.

[chapoton]

ping ? I have removed the controversial part..

EDIT: and this is an important ticket for progress in python3

[chapoton]

and bot is green

[chapoton]

please, someone ?

[tscrim]

LGTM.

[embray]

More list(map(...))s that might as well be list comprehensions:

src/sage/manifolds/differentiable/affine_connection.py

@@ -1186 +1186 @@
         if index_labels is None and isinstance(frame, CoordFrame) and \
           coordinate_labels:
             ch = frame.chart()
-            index_labels = map(str, ch[:])
-            index_latex_labels = map(latex, ch[:])
+            index_labels = list(map(str, ch[:]))
+            index_latex_labels = list(map(latex, ch[:]))
         return self.coef(frame=frame).display(symbol,
               latex_symbol=latex_symbol, index_positions='udd',
               index_labels=index_labels, index_latex_labels=index_latex_labels,

I think I've mentioned elsewhere (and forgive me if this concern was dismissed and I didn't see it), but I don't like these usesless hash example tests like:

src/sage/rings/fraction_field.py

@@ -688 +688 @@
         """
         return not (self == other)
 
+    def __hash__(self):
+        """
+        Compute the hash of ``self``.
+
+        EXAMPLES::
+
+            sage: h = hash(Frac(ZZ['x']))
+        """
+        return hash(self._R) ^ 147068341996611
+
     def ngens(self):
         """
         This is the same as for the parent object.

All this tests is that, at best, it doesn't crash (which is almost completely trivial). It should instead demonstrate at least a case where two hashes compare the same as expected, and where two hashes differ, as expected.

I agree it's annoying to write these tests which is why I've been pokey about making tickets for __hash__ implementations in the first place :)

[git]

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

​c91be02	Merge branch 'u/chapoton/25586' in 8.3.b7
​dab8b92	more doctests for hash

[chapoton]

done

[embray]

This is a stupid question and I think I might have already answered it in my head but, just for my edification:

src/sage/rings/fraction_field.py

@@ -688 +688 @@
         """
         return not (self == other)
 
+    def __hash__(self):
+        """
+        Compute the hash of ``self``.
+
+        EXAMPLES::
+
+            sage: h0 = hash(Frac(ZZ['x']))
+            sage: h1 = hash(Frac(ZZ['x']))
+            sage: h2 = hash(Frac(QQ['x']))
+            sage: h3 = hash(ZZ['x'])
+            sage: h0 == h1 and h1 != h2 and h1 != h3
+            True
+        """
+        return hash(self._R) ^ 147068341996611
+
     def ngens(self):
         """
         This is the same as for the parent object.

Why not just hash(self._R)? Is it possible for instances of these fields to have a parent ring that's equivalent to itself, or something (in which case we'd need this in order for them to have unique hashes)? And where does the magic number come from?

[chapoton]

This was modeled after the case of Laurent Polynomial at the request of Vincent, to avoid having trivially the same hash. The number is a random number.

[embray]

I see, he wrote:

For Laurent polynomial ring it is not a good idea to give it the same hash as the underlying multi-polynomial ring. You would better xor it with a random number

But why is that? It doesn't matter if they have the same hash unless they can also compare equal to each other.

In any case, even if there is a good reason, this should probably be noted. It's not obvious.

[embray]

My guess is that maybe you can have a polynomial ring over elements of another polynomial ring, so in that case it does make sense to do that.

[chapoton]

Do you want a note in the doc of the hash method ?

[embray]

In the doc or just in a comment. Up to you.

[git]

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

​6806c37

trac 25586 adding comment

[chapoton]

done

[chapoton]

ping ?

[chapoton]

PING ? This is a major hurting point..

[vdelecroix]

Replying to embray:

This is a stupid question and I think I might have already answered it in my head but, just for my edification:

src/sage/rings/fraction_field.py

@@ -688 +688 @@
         """
         return not (self == other)
 
+    def __hash__(self):
+        """
+        Compute the hash of ``self``.
+
+        EXAMPLES::
+
+            sage: h0 = hash(Frac(ZZ['x']))
+            sage: h1 = hash(Frac(ZZ['x']))
+            sage: h2 = hash(Frac(QQ['x']))
+            sage: h3 = hash(ZZ['x'])
+            sage: h0 == h1 and h1 != h2 and h1 != h3
+            True
+        """
+        return hash(self._R) ^ 147068341996611
+
     def ngens(self):
         """
         This is the same as for the parent object.

Why not just hash(self._R)?

To avoid collisions. We do have dictionaries whose keys are parent (e.g. coercions).

[vdelecroix]

For matter of equality, it is true that the polynomial ring on the zero ring is isomorphic to the base ring. Though, Sage does not know about it.

sage: S = Zmod(1)
sage: R = S['x']
sage: S == R
False

And even worse

sage: f = R.coerce_map_from(S)
sage: f.is_injective()
True
sage: f.is_surjective()
False

[vdelecroix]

([EDITED] see #25586 #25753 for the bug about surjectivity)

[tscrim]

Replying to vdelecroix:

(see #25586 for the bug about surjectivity)

See this ticket?

I am going to set this back to positive review as Erik's concerns have been addressed.

[vdelecroix]

Replying to tscrim:

Replying to vdelecroix:

(see #25586 for the bug about surjectivity)

See this ticket?

Oups: #25753

I am going to set this back to positive review as Erik's concerns have been addressed.

[embray]

Replying to chapoton:

PING ? This is a major hurting point..

Please try to show some patience. I have tickets that I believe are important that haven't been merged or even reviewed in months (a situation I'm not happy with, but there is only so much attention to go around).

If you've already fixed the issue then there's nothing holding you back--this is why I have my private Python 3 branch--everything I fix gets merged into it immediately and then I can carry on (even if that fix is not ultimately the accepted fix; at least it works in the meantime).

The rest of us have vacations, family, sickness, and other tasks to work on and adding "ping" to a ticket does not (at least in my case) make it any more likely that I'll see it sooner.