Sage: Ticket #25393: Removed unused and potentially misleading Morphism.__hash__
https://trac.sagemath.org/ticket/25393
<p>
As far as I can tell there is no code using the ability to hash <code>Morphisms</code>, and having this in the base class is misleading since there are subclasses which are not immutable and should not be hashable anyways. It would be better to implement <code>__hash__</code> on an as-needed basis for those classes that can guarantee immutability.
</p>
<h3 id="Originaldescription">Original description</h3>
<p>
<code>ContinuousMap</code> is impacted by <a class="closed ticket" href="https://trac.sagemath.org/ticket/24551" title="defect: py3: defining __eq__ breaks inheritance of __hash__ (closed: fixed)">#24551</a> in that it defines an <code>__eq__</code>, so its default hash is not inherited from its base class, <code>Morphism</code> (which does define <code>__hash__</code>).
</p>
<p>
This is one of the classes impacted by the issue raised <a class="ext-link" href="https://trac.sagemath.org/ticket/25387#comment:3"><span class="icon"></span>in this comment</a>.
</p>
<p>
I'm not sure what the best solution is. Right now I feel like it's somewhat ill-defined, because it's possible to have two <code>ContinuousMaps</code> that compare as equal, but have different hashes. That's not necessarily wrong, but it isn't obviously right either. If <code>x == y</code>, then <code>hash(x) == hash(y)</code> for a correct implementation. Technically I believe you can get away with the inverse--having <code>hash(x) == hash(y)</code> but <code>x != y</code>--but it is advisable for performance to avoid this.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/25393
Trac 1.1.6embrayFri, 18 May 2018 09:38:13 GMTdescription changed
https://trac.sagemath.org/ticket/25393#comment:1
https://trac.sagemath.org/ticket/25393#comment:1
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/25393?action=diff&version=1">diff</a>)
</li>
</ul>
TicketchapotonFri, 18 May 2018 09:48:47 GMTstatus, cc changed
https://trac.sagemath.org/ticket/25393#comment:2
https://trac.sagemath.org/ticket/25393#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_info</em>
</li>
<li><strong>cc</strong>
<em>egourgoulhon</em> added
</li>
</ul>
TickettscrimFri, 18 May 2018 16:05:28 GMT
https://trac.sagemath.org/ticket/25393#comment:3
https://trac.sagemath.org/ticket/25393#comment:3
<blockquote class="citation">
<p>
If two instances of the same class have the same hash, they must also be equal to each other with <code>==</code>
</p>
</blockquote>
<p>
That is true iff a perfect hash function exists, which it generally does not. In fact, that is <em>not</em> mandated by Python. For example, a valid hash function is 0 for all objects, but you loose performance benefits from <code>dict</code> because it becomes a list search.
</p>
<p>
The converse is really what you want: <code>==</code> implies same hash. (Also to be hashable, the hash should be immutable.) You can get away with it, but it causes problems when you try to put it into a set/dictionary as you get multiple instances of equal objects.
</p>
<p>
The issue with this class is that we have essentially a mutable element class. So I think the proper thing to do is just disable hashing. We do something similar for vectors and matrices.
</p>
TicketembrayFri, 18 May 2018 16:33:56 GMT
https://trac.sagemath.org/ticket/25393#comment:4
https://trac.sagemath.org/ticket/25393#comment:4
<p>
I think you must have misread me or something. I didn't mean that if two instances have the same hash it implies they are <code>==</code>. I mean that in order for hashing to be implemented correctly, then as you said if they are <code>==</code> they must have the same hash.
</p>
<p>
It actually wasn't immediately obvious to me that this class is not immutable. Where can it be mutated? All I noticed is that you define two <code>ContinuousMaps</code> that are the same except that one is constructed with <code>is_isomorphism=True</code> and the other with <code>is_isomorphism=False</code>. In this case they are <code>==</code> but have different hashes.
</p>
TickettscrimFri, 18 May 2018 16:48:45 GMTdescription changed
https://trac.sagemath.org/ticket/25393#comment:5
https://trac.sagemath.org/ticket/25393#comment:5
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/25393?action=diff&version=5">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:4" title="Comment 4">embray</a>:
</p>
<blockquote class="citation">
<p>
I think you must have misread me or something. I didn't mean that if two instances have the same hash it implies they are <code>==</code>. I mean that in order for hashing to be implemented correctly, then as you said if they are <code>==</code> they must have the same hash.
</p>
</blockquote>
<p>
Paraphrasing what you wrote: if <code>x.__class__ == y.__class__</code> and <code>hash(x) == hash(y)</code>, then <code>x == y</code>. It is a bad smell for <code>x == y</code> with <code>hash(x) == hash(y)</code>. We're on the same page with what needs to be done, but I just saying the ticket description needed an update, which I have just done.
</p>
<p>
IMO, it is not a code smell to have <code>hash(x) == hash(y)</code> but <code>x != y</code>. However, I do agree that it is better to have a more perfect hash function, but there are only so many hash values since hashes are limited to 64 bit integers (unless I am misunderstanding something in Python).
</p>
<blockquote class="citation">
<p>
It actually wasn't immediately obvious to me that this class is not immutable. Where can it be mutated? All I noticed is that you define two <code>ContinuousMaps</code> that are the same except that one is constructed with <code>is_isomorphism=True</code> and the other with <code>is_isomorphism=False</code>. In this case they are <code>==</code> but have different hashes.
</p>
</blockquote>
<p>
See <code>add_expr</code> and <code>set_expr</code>.
</p>
TicketembrayMon, 21 May 2018 11:24:23 GMT
https://trac.sagemath.org/ticket/25393#comment:6
https://trac.sagemath.org/ticket/25393#comment:6
<p>
Well in that case this is already fixed for free on Python 3 since it sets <code>__hash__ = None</code> automatically for any class that defines <code>__eq__</code> but not <code>__hash__</code>, and this should just be done explicitly for this class.
</p>
<p>
In general should <code>Morphism</code> even have a default hash? It doesn't seem that immediately useful to me.
</p>
TickettscrimMon, 21 May 2018 13:30:04 GMT
https://trac.sagemath.org/ticket/25393#comment:7
https://trac.sagemath.org/ticket/25393#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:6" title="Comment 6">embray</a>:
</p>
<blockquote class="citation">
<p>
Well in that case this is already fixed for free on Python 3 since it sets <code>__hash__ = None</code> automatically for any class that defines <code>__eq__</code> but not <code>__hash__</code>, and this should just be done explicitly for this class.
</p>
</blockquote>
<p>
I agree. That is probably the correct thing to for this class.
</p>
<blockquote class="citation">
<p>
In general should <code>Morphism</code> even have a default hash? It doesn't seem that immediately useful to me.
</p>
</blockquote>
<p>
I cannot think of a general mechanism that would require morphisms to be keys to a cache. Ideally matrices should be a subclass of <code>Morphism</code>, which would break the implicit <code>Morphism</code> is meant to be immutable. Although the fact that a <code>Morphism</code> can be stored via a coercion might lead to subtle bugs when it is mutable.
</p>
TicketembrayMon, 21 May 2018 13:51:13 GMT
https://trac.sagemath.org/ticket/25393#comment:8
https://trac.sagemath.org/ticket/25393#comment:8
<p>
Okay then, I think I'll just try removing <code>Morphism.__hash__</code>. Neither the commit that added it, nor the associated ticket (<a class="closed ticket" href="https://trac.sagemath.org/ticket/13214" title="enhancement: Finite field homomorphisms and Frobenius endomorphisms (closed: fixed)">#13214</a>) provide any explanation for <em>why</em> it was added. It just was.
</p>
<p>
I don't think we should be adding <code>__hash__</code> implementations anywhere that it isn't explicitly needed (or at least a specific use case is being imagined). Especially not for base classes that could have mutable subclasses (one could make exceptions of course but in that case it would also be good to have an explicit mechanism for marking whether or not instances of some class are expected to be immutable).
</p>
TicketembrayMon, 21 May 2018 15:44:42 GMT
https://trac.sagemath.org/ticket/25393#comment:9
https://trac.sagemath.org/ticket/25393#comment:9
<p>
<del>Oh fun. Deleting <code>Morphism.__hash__</code> causes segfaults :)</del> No, I was just misreading the test output. It wasn't segfaults, just timeouts (the stack traces threw me off). Still I wonder if that was a regression...
</p>
TicketembrayMon, 21 May 2018 16:51:57 GMTstatus, description, summary changed; author, branch, commit set
https://trac.sagemath.org/ticket/25393#comment:10
https://trac.sagemath.org/ticket/25393#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/25393?action=diff&version=10">diff</a>)
</li>
<li><strong>author</strong>
set to <em>Erik Bray</em>
</li>
<li><strong>summary</strong>
changed from <em>Fix hash for ContinuousMap</em> to <em>Removed unused and potentially misleading Morphism.__hash__</em>
</li>
<li><strong>branch</strong>
set to <em>u/embray/misc/ticket-25393</em>
</li>
<li><strong>commit</strong>
set to <em>1a990578b1b8f0826fa0408aaac424b205f5dffb</em>
</li>
</ul>
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=1a990578b1b8f0826fa0408aaac424b205f5dffb"><span class="icon"></span>1a99057</a></td><td><code>remove unused and potentially misleading __hash__ implementation for Morphisms</code>
</td></tr></table>
TicketembrayMon, 21 May 2018 16:59:32 GMTstatus changed
https://trac.sagemath.org/ticket/25393#comment:11
https://trac.sagemath.org/ticket/25393#comment:11
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Looks like this has some test failures. It's possible that in some of these cases the real problem is they shouldn't use <code>UniqueRepresentation</code> in the first place, but I'm not sure.
</p>
TicketchapotonTue, 22 May 2018 14:30:00 GMT
https://trac.sagemath.org/ticket/25393#comment:12
https://trac.sagemath.org/ticket/25393#comment:12
<p>
bot says "no failing doctest"..
</p>
TicketembrayTue, 22 May 2018 14:38:09 GMT
https://trac.sagemath.org/ticket/25393#comment:13
https://trac.sagemath.org/ticket/25393#comment:13
<p>
I'll have to try again. I got some failures when I ran it, but it might have been a matter of running the tests out of their "normal" order, and hence weird things happening with the coercion framework or something...
</p>
TicketegourgoulhonWed, 30 May 2018 16:14:56 GMT
https://trac.sagemath.org/ticket/25393#comment:14
https://trac.sagemath.org/ticket/25393#comment:14
<p>
Sorry for showing up late in the discussion, but for some reason, I did not received the Trac notification that I've been put in the Cc list of this ticket; I actually discovered the existence of the ticket only today, while reading the "python3 status" post on sage-devel...
</p>
<p>
Let me summarize a few things about <code>ContinuousMap</code>:
</p>
<ul><li><code>ContinousMap</code> objects are "in spirit" immutable: they represent continuous maps between topological manifolds and once such a map is defined, there is no meaning in modifying it. However, in practice, such maps cannot be fully defined at their creation by the user, because some new charts may be introduced on the fly on the manifold and a given map should be able to have coordinate expressions in terms of these charts, hence the methods <code>add_expr</code> and <code>set_expr</code>.
</li></ul><ul><li><code>ContinousMap</code> (more precisely the derived class <code>DiffMap</code>) needs to be hashable because <code>DiffMap</code> objects serve as keys in the dictionary of vector field modules on a manifold (see <a class="ext-link" href="http://doc.sagemath.org/html/en/reference/manifolds/sage/manifolds/differentiable/vectorfield_module.html"><span class="icon"></span>here</a> for details).
</li></ul><ul><li>I understand that the <code>__hash__</code> method inherited from <code>Morphism</code> relies on the <code>repr</code> of the continuous map and in some cases (maps <code>f</code> and <code>g</code> having the same domain, codomain, but different names given by the user), one may have <code>f == g</code> with <code>hash(f) != hash(g)</code>, which is very bad!
</li></ul><p>
I propose to implement a <code>__hash__</code> method in <code>ContinuousMap</code> that ensures that maps that compare equal have the same hash. It would make sense that I do this atop commit 1a9907 (i.e. the commit in the branch currently attached to this ticket), since the latter is suppressing <code>Morphism.__hash__</code>. Do you agree?
</p>
TicketembrayThu, 31 May 2018 10:22:39 GMTowner set
https://trac.sagemath.org/ticket/25393#comment:15
https://trac.sagemath.org/ticket/25393#comment:15
<ul>
<li><strong>owner</strong>
changed from <em>(none)</em> to <em>egourgoulhon</em>
</li>
</ul>
<p>
Thanks Eric for the explanation. I'm a bit iffy on the first point--it's a bit unfortunate, but also an implementation detail--I don't feel qualified to fuss about it right now.
</p>
<p>
Based on your explanation though, maybe I should back off a bit on my argument that <code>Morphism.__hash__</code> should be removed (unless someone can point out other subclasses of it that are definitely <em>not</em> in any sense immutable). Instead, I might just change the implementation a bit to do away with its use of <code>repr()</code>. If the domain doesn't have generators we can just ignore that and use <code>None</code> or something. IMO it's the <code>repr()</code> that's the real problem because that's just flakey and surprising. So maybe I'll just change that for now and see what that affects.
</p>
<p>
Meanwhile, yes, if you think you know the best way to implement a <code>ContinuousMap.__hash__</code> please do. Please also make sure there is a test demonstrating not only that the hash works, but <em>why</em> it should work, since my experiment in removing it didn't appear to break any test after all...
</p>
TicketegourgoulhonThu, 31 May 2018 11:54:50 GMT
https://trac.sagemath.org/ticket/25393#comment:16
https://trac.sagemath.org/ticket/25393#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:15" title="Comment 15">embray</a>:
</p>
<blockquote class="citation">
<p>
Thanks Eric for the explanation. I'm a bit iffy on the first point--it's a bit unfortunate, but also an implementation detail--I don't feel qualified to fuss about it right now.
</p>
</blockquote>
<p>
Actually the type of immutatibility involved for <code>ContinuousMap</code> is that defined by Simon King in this <a class="ext-link" href="https://groups.google.com/d/msg/sage-devel/d8-r3uEewFU/Jb02875ACgAJ"><span class="icon"></span>sage-devel post</a>: methods, like <code>add_expr</code>, may change (enhance) the internal representation of the object, but in any case they must preserve the equivalence class with respect to <code>==</code> to which the object belongs at its creation.
</p>
<blockquote class="citation">
<p>
Based on your explanation though, maybe I should back off a bit on my argument that <code>Morphism.__hash__</code> should be removed (unless someone can point out other subclasses of it that are definitely <em>not</em> in any sense immutable). Instead, I might just change the implementation a bit to do away with its use of <code>repr()</code>. If the domain doesn't have generators we can just ignore that and use <code>None</code> or something. IMO it's the <code>repr()</code> that's the real problem because that's just flakey and surprising. So maybe I'll just change that for now and see what that affects.
</p>
</blockquote>
<p>
Indeed, for <code>ContinousMap</code>, which don't have any "generator", the issue is the default back to the hash of <code>repr()</code>.
</p>
<blockquote class="citation">
<p>
Meanwhile, yes, if you think you know the best way to implement a <code>ContinuousMap.__hash__</code> please do.
</p>
</blockquote>
<p>
OK I will do it.
</p>
<blockquote class="citation">
<p>
Please also make sure there is a test demonstrating not only that the hash works, but <em>why</em> it should work, since my experiment in removing it didn't appear to break any test after all...
</p>
</blockquote>
<p>
I am quite surprised about this: how could <code>ContinuousMap</code> objects be used as dictionary keys without any <code>__hash__</code> method?
</p>
TicketembrayThu, 31 May 2018 12:05:23 GMT
https://trac.sagemath.org/ticket/25393#comment:17
https://trac.sagemath.org/ticket/25393#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:16" title="Comment 16">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:15" title="Comment 15">embray</a>:
</p>
<blockquote class="citation">
<p>
Please also make sure there is a test demonstrating not only that the hash works, but <em>why</em> it should work, since my experiment in removing it didn't appear to break any test after all...
</p>
</blockquote>
<p>
I am quite surprised about this: how could <code>ContinuousMap</code> objects be used as dictionary keys without any <code>__hash__</code> method?
</p>
</blockquote>
<p>
I don't know. Are there definitely examples of this in the tests? I <em>did</em> get some test failures when I tried this the first time, but the patchbot shows tests passing. May it's just in some <code>--long</code> tests?
</p>
TicketegourgoulhonThu, 31 May 2018 12:27:32 GMT
https://trac.sagemath.org/ticket/25393#comment:18
https://trac.sagemath.org/ticket/25393#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:16" title="Comment 16">egourgoulhon</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Please also make sure there is a test demonstrating not only that the hash works, but <em>why</em> it should work, since my experiment in removing it didn't appear to break any test after all...
</p>
</blockquote>
<p>
I am quite surprised about this: how could <code>ContinuousMap</code> objects be used as dictionary keys without any <code>__hash__</code> method?
</p>
</blockquote>
<p>
I did the experiment, i.e. remove <code>Morphism.__hash__</code>, and discovered that the hash of <code>ContinousMap</code> is then inherited from <code>Map.__hash__</code>, which does (cf. line 1292 o f <code>src/sage/categories/map.pyx</code>):
</p>
<pre class="wiki">return hash((self.domain(), self._codomain))
</pre><p>
This simple hash seems very satisfactory to me, except maybe for <code>self._codomain</code> that may be replaced by <code>self.codomain()</code>. Actually this is what I had in mind when proposing to implement <code>ContinousMap.__hash__</code>. So if you remove <code>Morphism.__hash__</code>, there is no need to write a proper <code>ContinousMap.__hash__</code>: that of <code>Map</code> is sufficient.
</p>
TickettscrimThu, 31 May 2018 13:50:48 GMT
https://trac.sagemath.org/ticket/25393#comment:19
https://trac.sagemath.org/ticket/25393#comment:19
<p>
I would probably also consider removing <code>Map.__hash__</code> (and reimplementing that in <code>ContinuousMap</code>) for the same reasons as removing <code>Morphism.__hash__</code>.
</p>
TicketegourgoulhonThu, 31 May 2018 21:38:06 GMT
https://trac.sagemath.org/ticket/25393#comment:20
https://trac.sagemath.org/ticket/25393#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:19" title="Comment 19">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I would probably also consider removing <code>Map.__hash__</code> (and reimplementing that in <code>ContinuousMap</code>) for the same reasons as removing <code>Morphism.__hash__</code>.
</p>
</blockquote>
<p>
Yes. Anyway, if I understand correctly, for Python3, one needs to implement <code>ContinuousMap.__hash__</code> because there is a <code>ContinuousMap.__eq__</code>, so that <code>__hash__</code> cannot be inherited. Correct?
</p>
<p>
Btw, other manifolds classes that have to be hashable because they serve as dictionary keys are <code>TopologicalManifold</code>, <code>Chart</code> and <code>VectorFrame</code>. However, they all inherit from <code>UniqueRepresentation</code>, from which they get <code>__hash__</code> and <code>__eq__</code>. So it should be OK for Python3 with them, shouldn't it?
</p>
TicketegourgoulhonFri, 01 Jun 2018 12:18:31 GMT
https://trac.sagemath.org/ticket/25393#comment:21
https://trac.sagemath.org/ticket/25393#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:19" title="Comment 19">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I would probably also consider removing <code>Map.__hash__</code> (and reimplementing that in <code>ContinuousMap</code>) for the same reasons as removing <code>Morphism.__hash__</code>.
</p>
</blockquote>
<p>
Removing <code>Map.__hash__</code>, <code>FormalCompositeMap.__hash__</code> (to be coherent with the removal of <code>Map.__hash__</code>) and <code>Morphism.__hash__</code>, while implementing <code>ContinuousMap.__hash__</code>, leads to (only) two doctest errors in all Sage 8.3.beta3, as reported by <code>make ptestlong</code>:
</p>
<pre class="wiki">sage -t --long --warn-long 53.5 src/sage/combinat/root_system/hecke_algebra_representation.py
**********************************************************************
File "src/sage/combinat/root_system/hecke_algebra_representation.py", line 683, in sage.combinat.root_system.hecke_algebra_representation.HeckeAlgebraRepresentation._test_Y
Failed example:
rho._test_Y() # long time (4s)
...
TypeError: <class 'sage.modules.with_basis.morphism.ModuleMorphismByLinearity_with_category'> is not hashable
</pre><p>
and
</p>
<pre class="wiki">sage -t --long --warn-long 53.5 src/sage/groups/semimonomial_transformations/semimonomial_transformation.pyx
**********************************************************************
File "src/sage/groups/semimonomial_transformations/semimonomial_transformation.pyx", line 170, in sage.groups.semimonomial_transformations.semimonomial_transformation.SemimonomialTransformation.__hash__
Failed example:
hash( SemimonomialTransformationGroup(F, 4).an_element() ) #random #indirect doctest
...
TypeError: <type 'sage.rings.finite_rings.hom_finite_field.FiniteFieldHomomorphism_generic'> is not hashable
</pre>
TicketembrayFri, 01 Jun 2018 22:19:10 GMT
https://trac.sagemath.org/ticket/25393#comment:22
https://trac.sagemath.org/ticket/25393#comment:22
<p>
Those tests would just have to be removed I guess. Though I'm still not 100% sure now that we even want to outright remove <code>Map.__hash__</code>. But maybe you actually agree with my initial thought to remove it.
</p>
TicketnbruinFri, 01 Jun 2018 23:29:59 GMT
https://trac.sagemath.org/ticket/25393#comment:23
https://trac.sagemath.org/ticket/25393#comment:23
<p>
Slightly more elegant than removing tests might be by installing a reasonable hash. For:
'sage.rings.finite_rings.hom_finite_field.FiniteFieldHomomorphism_generic'
I would think that
<code> hash((f.domain(),f.codomain())+tuple(f.im_gens()))</code>
might be a reasonable option.
</p>
<p>
and for the second one
<code>hash((phi.domain(),phi.codomain(),tuple(phi(x) for x in X.basis())))</code>
</p>
<p>
is probably OK. These are both homomorphisms on finitely generated structures, so equality and hashing are relatively straightforward (if they are implemented for elements of the codomain).
</p>
TicketegourgoulhonSun, 03 Jun 2018 20:17:59 GMT
https://trac.sagemath.org/ticket/25393#comment:24
https://trac.sagemath.org/ticket/25393#comment:24
<p>
Finally, I've implemented <code>ContinousMap.__hash__</code> in a separate ticket: <a class="closed ticket" href="https://trac.sagemath.org/ticket/25502" title="defect: py3: implement __hash__ method in ContinuousMap (closed: fixed)">#25502</a>, in order to have a quick fix for the Python3 issue.
</p>
TicketembrayWed, 18 Jul 2018 11:47:03 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:25
https://trac.sagemath.org/ticket/25393#comment:25
<ul>
<li><strong>milestone</strong>
changed from <em>sage-8.3</em> to <em>sage-8.4</em>
</li>
</ul>
<p>
I believe this issue can reasonably be addressed for Sage 8.4.
</p>
TicketembrayThu, 06 Sep 2018 13:51:19 GMTstatus changed
https://trac.sagemath.org/ticket/25393#comment:26
https://trac.sagemath.org/ticket/25393#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
</ul>
<p>
Looking back at <a class="closed ticket" href="https://trac.sagemath.org/ticket/25502" title="defect: py3: implement __hash__ method in ContinuousMap (closed: fixed)">#25502</a>, and looking the comments in this ticket again, was the hash added in <a class="closed ticket" href="https://trac.sagemath.org/ticket/25502" title="defect: py3: implement __hash__ method in ContinuousMap (closed: fixed)">#25502</a> really necessary? If we still go with removing the existing <code>Morphism.__hash__</code> (should we?) then <code>ContinuousMap</code> inherits <code>Map.__hash__</code> which is identical to the <code>ContinuousMap.__hash__</code> added in <a class="closed ticket" href="https://trac.sagemath.org/ticket/25502" title="defect: py3: implement __hash__ method in ContinuousMap (closed: fixed)">#25502</a>. Should <code>Map.__hash__</code> also be removed?
</p>
TicketegourgoulhonSun, 09 Sep 2018 15:42:08 GMT
https://trac.sagemath.org/ticket/25393#comment:27
https://trac.sagemath.org/ticket/25393#comment:27
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:26" title="Comment 26">embray</a>:
</p>
<blockquote class="citation">
<p>
Looking back at <a class="closed ticket" href="https://trac.sagemath.org/ticket/25502" title="defect: py3: implement __hash__ method in ContinuousMap (closed: fixed)">#25502</a>, and looking the comments in this ticket again, was the hash added in <a class="closed ticket" href="https://trac.sagemath.org/ticket/25502" title="defect: py3: implement __hash__ method in ContinuousMap (closed: fixed)">#25502</a> really necessary? If we still go with removing the existing <code>Morphism.__hash__</code> (should we?) then <code>ContinuousMap</code> inherits <code>Map.__hash__</code> which is identical to the <code>ContinuousMap.__hash__</code> added in <a class="closed ticket" href="https://trac.sagemath.org/ticket/25502" title="defect: py3: implement __hash__ method in ContinuousMap (closed: fixed)">#25502</a>. Should <code>Map.__hash__</code> also be removed?
</p>
</blockquote>
<p>
Indeed <code>ContinuousMap.__hash__</code> and <code>Map.__hash__</code> are essentially identical, but I guess the logic of removing <code>Morphism.__hash__</code> applies to <code>Map.__hash__</code> as well. From the ticket description: <em>As far as I can tell there is no code using the ability to hash Morphisms, and having this in the base class is misleading since there are subclasses which are not immutable and should not be hashable anyways. It would be better to implement <span class="underline">hash</span> on an as-needed basis for those classes that can guarantee immutability.</em> If only <code>Morphism.__hash__</code> is removed and <code>Map.__hash__</code> is kept, then all subclasses will still have some <code>__hash__</code> inherited from their base class (<code>Map</code>) and therefore will look hashable to Python3, even if they are not immutable, which is truly dangerous...
</p>
<p>
PS: I realize that the removing of <code>Map.__hash__</code> was already advocated by Travis for the very same reasons in <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:19" title="Comment 19">comment:19</a>. See also <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:21" title="Comment 21">comment:21</a> for the consequences of the removal of <code>Map.__hash__</code> (only 2 failed doctests).
</p>
TicketegourgoulhonSun, 09 Sep 2018 16:22:06 GMT
https://trac.sagemath.org/ticket/25393#comment:28
https://trac.sagemath.org/ticket/25393#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25393#comment:27" title="Comment 27">egourgoulhon</a>:
</p>
<blockquote class="citation">
<p>
If only <code>Morphism.__hash__</code> is removed and <code>Map.__hash__</code> is kept, then all subclasses will still have some <code>__hash__</code> inherited from their base class (<code>Map</code>) and therefore will look hashable to Python3, even if they are not immutable, which is truly dangerous...
</p>
</blockquote>
<p>
Actually I was wrong: there is no danger since in Python3, a class that defines some <code>__eq__</code> does no longer inherit <code>__hash__</code> from any base class. From <a class="ext-link" href="https://docs.python.org/3/reference/datamodel.html#object.__hash__"><span class="icon"></span>this part of Python 3 reference</a>: <em>If a class that overrides <code>__eq__()</code> needs to retain the implementation of <code>__hash__()</code> from a parent class, the interpreter must be told this explicitly by setting <code>__hash__ = <ParentClass>.__hash__</code>.</em> In our case, this means that if one does not write explicitly
</p>
<pre class="wiki"> __hash__ = Map.__hash__
</pre><p>
(and of course does not reimplement <code>__hash__</code>), then the subclass is not hashable.
</p>
<p>
Probably things will be more clear if <code>Map.__hash__</code> is removed.
</p>
TicketembraySun, 28 Oct 2018 11:41:42 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:29
https://trac.sagemath.org/ticket/25393#comment:29
<ul>
<li><strong>milestone</strong>
changed from <em>sage-8.4</em> to <em>sage-8.5</em>
</li>
</ul>
TicketembrayFri, 28 Dec 2018 14:10:15 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:30
https://trac.sagemath.org/ticket/25393#comment:30
<ul>
<li><strong>milestone</strong>
changed from <em>sage-8.5</em> to <em>sage-8.7</em>
</li>
</ul>
<p>
Retargeting some of my tickets (somewhat optimistically for now).
</p>
TicketembrayMon, 25 Mar 2019 10:43:18 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:31
https://trac.sagemath.org/ticket/25393#comment:31
<ul>
<li><strong>milestone</strong>
changed from <em>sage-8.7</em> to <em>sage-8.8</em>
</li>
</ul>
<p>
Moving all my in-progress tickets to 8.8 milestone.
</p>
TicketembrayFri, 14 Jun 2019 14:50:27 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:32
https://trac.sagemath.org/ticket/25393#comment:32
<ul>
<li><strong>milestone</strong>
changed from <em>sage-8.8</em> to <em>sage-8.9</em>
</li>
</ul>
<p>
Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).
</p>
TicketembrayMon, 30 Dec 2019 14:48:17 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:33
https://trac.sagemath.org/ticket/25393#comment:33
<ul>
<li><strong>milestone</strong>
changed from <em>sage-8.9</em> to <em>sage-9.1</em>
</li>
</ul>
<p>
Ticket retargeted after milestone closed
</p>
TicketmkoeppeFri, 01 May 2020 04:28:42 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:34
https://trac.sagemath.org/ticket/25393#comment:34
<ul>
<li><strong>milestone</strong>
changed from <em>sage-9.1</em> to <em>sage-9.2</em>
</li>
</ul>
<p>
Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.
</p>
TicketmkoeppeSat, 24 Oct 2020 20:15:01 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:35
https://trac.sagemath.org/ticket/25393#comment:35
<ul>
<li><strong>milestone</strong>
changed from <em>sage-9.2</em> to <em>sage-9.3</em>
</li>
</ul>
TicketmkoeppeMon, 10 May 2021 17:42:09 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:36
https://trac.sagemath.org/ticket/25393#comment:36
<ul>
<li><strong>milestone</strong>
changed from <em>sage-9.3</em> to <em>sage-9.4</em>
</li>
</ul>
<p>
Moving to 9.4, as 9.3 has been released.
</p>
TicketmkoeppeMon, 19 Jul 2021 00:44:56 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:37
https://trac.sagemath.org/ticket/25393#comment:37
<ul>
<li><strong>milestone</strong>
changed from <em>sage-9.4</em> to <em>sage-9.5</em>
</li>
</ul>
<p>
Setting a new milestone for this ticket based on a cursory review.
</p>
TicketmkoeppeSat, 18 Dec 2021 19:53:12 GMTmilestone changed
https://trac.sagemath.org/ticket/25393#comment:38
https://trac.sagemath.org/ticket/25393#comment:38
<ul>
<li><strong>milestone</strong>
changed from <em>sage-9.5</em> to <em>sage-9.6</em>
</li>
</ul>
<p>
Stalled in <code>needs_review</code> or <code>needs_info</code>; likely won't make it into Sage 9.5.
</p>
Ticket