Sage: Ticket #25946: py3: fixes for sage.schemes.hyperelliptic_curves
https://trac.sagemath.org/ticket/25946
<p>
The main thing needed here was a <code>__hash__</code>.
Or removing <code>__eq__</code> and <code>__ne__</code>
</p>
<p>
Part of <a class="closed ticket" href="https://trac.sagemath.org/ticket/24551" title="defect: py3: defining __eq__ breaks inheritance of __hash__ (closed: fixed)">#24551</a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/25946
Trac 1.1.6embrayFri, 27 Jul 2018 10:27:25 GMTstatus changed
https://trac.sagemath.org/ticket/25946#comment:1
https://trac.sagemath.org/ticket/25946#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TickettscrimFri, 27 Jul 2018 12:53:38 GMTstatus changed; keywords, reviewer set
https://trac.sagemath.org/ticket/25946#comment:2
https://trac.sagemath.org/ticket/25946#comment:2
<ul>
<li><strong>keywords</strong>
<em>sagedays@icerm</em> added
</li>
<li><strong>reviewer</strong>
set to <em>Travis Scrimshaw</em>
</li>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
You have too much data in your <code>__hash__</code>:
</p>
<pre class="wiki">sage: P.<x> = QQ[]
sage: f0 = 4*x^5 - 30*x^3 + 45*x - 22
sage: C0 = HyperellipticCurve(f0)
sage: P.<x> = RR[]
sage: f1 = 4*x^5 - 30*x^3 + 45*x - 22
sage: C1 = HyperellipticCurve(f1)
sage: C1 == C0
True
sage: hash(C1) == hash(C0)
False
</pre><p>
So you definitely have to remove <code>_PP</code> from the hash, and I would probably remove both the <code>__class__</code> as subclasses are not used in the <code>__eq__</code> comparison.
</p>
TicketembrayFri, 27 Jul 2018 14:17:44 GMT
https://trac.sagemath.org/ticket/25946#comment:3
https://trac.sagemath.org/ticket/25946#comment:3
<p>
I'm not 100% sure about that. I'd need to double-check what the intention is here. On Python 2 it was inheriting the flimsy default <code>__hash__</code> from <code>CategoryObject</code> that just hashes its <code>__repr__</code>. So for this case I was including essentially the same data in the hash that would be needed to differentiate the reprs of two of these objects.
</p>
<p>
But maybe that's not necessary. It just wasn't clear what the intent was (if any) due to the default hash...
</p>
TicketembrayFri, 27 Jul 2018 14:17:57 GMTstatus changed
https://trac.sagemath.org/ticket/25946#comment:4
https://trac.sagemath.org/ticket/25946#comment:4
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
</ul>
TickettscrimFri, 27 Jul 2018 14:41:08 GMT
https://trac.sagemath.org/ticket/25946#comment:5
https://trac.sagemath.org/ticket/25946#comment:5
<p>
I completely agree the <code>repr</code> hash is bad. I think the data used for equality should be what is used for the <code>__hash__</code>, not more. From what you're saying, it seems like we are actually fixing a bug if we only hash <code>self._hyperelliptic_polynomials</code>.
</p>
<p>
My understanding behind the default hash is that every object has a <code>repr</code> and generally two objects that compare equal have equal <code>repr</code> output.
</p>
<p>
I don't use/know this code, so we probably have to ask someone who understands it better to confirm if you're worried enough.
</p>
TicketembrayFri, 27 Jul 2018 14:43:28 GMT
https://trac.sagemath.org/ticket/25946#comment:6
https://trac.sagemath.org/ticket/25946#comment:6
<p>
I could certainly try a simpler repr to start with and see what happens. If that works, then I agree with you we should just use what <code>__eq__</code> compares.
</p>
TickettscrimFri, 27 Jul 2018 14:45:00 GMT
https://trac.sagemath.org/ticket/25946#comment:7
https://trac.sagemath.org/ticket/25946#comment:7
<p>
Sounds good. Thanks.
</p>
TicketchapotonFri, 10 Aug 2018 07:19:57 GMT
https://trac.sagemath.org/ticket/25946#comment:8
https://trac.sagemath.org/ticket/25946#comment:8
<p>
any progress here ?
</p>
TicketembrayFri, 10 Aug 2018 09:30:25 GMT
https://trac.sagemath.org/ticket/25946#comment:9
https://trac.sagemath.org/ticket/25946#comment:9
<p>
I mean, clearly not. I'd prefer if someone who knew this code were to chime in, but otherwise I'll get to it if/when I feel like taking the time to understand this code better.
</p>
TicketembrayFri, 10 Aug 2018 09:33:03 GMT
https://trac.sagemath.org/ticket/25946#comment:10
https://trac.sagemath.org/ticket/25946#comment:10
<p>
To be clear, I also agree with everything Travis wrote on this ticket; I'm just not in a rush to act on it because I don't feel qualified to make that judgment call w.r.t. this code.
</p>
TickettscrimFri, 10 Aug 2018 09:52:02 GMTcc set
https://trac.sagemath.org/ticket/25946#comment:11
https://trac.sagemath.org/ticket/25946#comment:11
<ul>
<li><strong>cc</strong>
<em>bhutz</em> added
</li>
</ul>
<p>
Ben, do you know about this code or know who we should cc?
</p>
TicketbhutzFri, 10 Aug 2018 13:03:27 GMTcc changed
https://trac.sagemath.org/ticket/25946#comment:12
https://trac.sagemath.org/ticket/25946#comment:12
<ul>
<li><strong>cc</strong>
<em>cremona</em> added
</li>
</ul>
<p>
I'm not familiar with this code. John may know.
</p>
TicketcremonaFri, 10 Aug 2018 14:07:23 GMT
https://trac.sagemath.org/ticket/25946#comment:13
https://trac.sagemath.org/ticket/25946#comment:13
<p>
I don't know or use the code (except perhaps occasionally) and so I do not really understand the issue here. I don't think that I have ever written such a has function so do not know what properties it is supposed to have.
</p>
<p>
What exactly was the problem which this patch is intended to fix?
</p>
TicketalexjbestFri, 10 Aug 2018 15:45:38 GMT
https://trac.sagemath.org/ticket/25946#comment:14
https://trac.sagemath.org/ticket/25946#comment:14
<p>
I'm really confused about whether or not we should call these hyperelliptic curves equal in the first place. The current code returns true because the polynomials are equal which is because both variables are named <code>x</code> even though they are over different fields, are two curves over different fields really _equal_, even if one is just a base change of the other? For contrast
</p>
<pre class="wiki">sage: P.<x,y>= RR[]
sage: C0 = Curve(y - x^2 - 1)
sage: C0
Affine Plane Curve over Real Field with 53 bits of precision defined by -x^2 + y - 1.00000000000000
sage: P.<x,y>= QQ[]
sage: C1 = Curve(y - x^2 - 1)
sage: C1
Affine Plane Curve over Rational Field defined by -x^2 + y - 1
sage: C0 == C1
False
</pre>
TicketembrayFri, 10 Aug 2018 15:59:10 GMT
https://trac.sagemath.org/ticket/25946#comment:15
https://trac.sagemath.org/ticket/25946#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:14" title="Comment 14">alexjbest</a>:
</p>
<blockquote class="citation">
<p>
I'm really confused about whether or not we should call these hyperelliptic curves equal in the first place. The current code returns true because the polynomials are equal which is because both variables are named <code>x</code> even though they are over different fields, are two curves over different fields really _equal_, even if one is just a base change of the other?
</p>
</blockquote>
<p>
I had the same doubt before. There's some inconsistency throughout Sage about when some objects defined over different fields are considered equal under <code>==</code>. In some cases it's quite deliberate, in other cases it's not clear until and unless the original author chimes in. If the author isn't even sure I would lean against calling them <code>==</code>, much less having the same hash.
</p>
TicketcremonaFri, 10 Aug 2018 16:11:19 GMT
https://trac.sagemath.org/ticket/25946#comment:16
https://trac.sagemath.org/ticket/25946#comment:16
<p>
I agree with Alex: to be equal the fields of definition should equal and the defining polynomials identical. This is the case with elliptic curves:
</p>
<pre class="wiki">sage: E = EllipticCurve([0,1])
sage: E
Elliptic Curve defined by y^2 = x^3 + 1 over Rational Field
sage: K.<i> = NumberField(x^2+1)
sage: EK = E.change_ring(K)
sage: EK
Elliptic Curve defined by y^2 = x^3 + 1 over Number Field in i with defining polynomial x^2 + 1
sage: E == EK
False
sage: hash(E)
8741953973726
sage: hash(EK)
8741951786400
</pre><p>
I don't know what this hash function actually does: it calls hash_by_id, whatever that is. I also don't know how to find out what function is being called by E==EK either!
</p>
TickettscrimFri, 10 Aug 2018 23:17:52 GMT
https://trac.sagemath.org/ticket/25946#comment:17
https://trac.sagemath.org/ticket/25946#comment:17
<p>
<code>EllipticCurve</code> is a <code>UniqueFactory</code>, so the input (i.e., in part, the field) uniquely identifies the curve. So the <code>hash</code> and <code>==</code> is obtained by using <code>id</code> as there is a unique such object in memory at a given time. Changing the hyperelliptic to suppose the <code>UniqueRepresentation</code>-type behavior would be a very invasive and complicated surgery I think.
</p>
<p>
So the short-term fix IMO would be deciding an appropriate notion of <code>__eq__</code>, and then changing the <code>__hash__</code> to match. From what John is saying, we should also introduce a check that the defining fields are equal.
</p>
<p>
Also, from <code>git blame</code>, "Nick Alexander" and "tornaria" comes up a lot and David Kohel is listed in the copyright.
</p>
TicketchapotonTue, 14 Aug 2018 08:05:55 GMTstatus, commit, branch changed
https://trac.sagemath.org/ticket/25946#comment:18
https://trac.sagemath.org/ticket/25946#comment:18
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
changed from <em>cb04ceb814b625383bce962fed09e39e7fa04184</em> to <em>78ae9220be1f5279f1766ec78f597bb4634b69e2</em>
</li>
<li><strong>branch</strong>
changed from <em>u/embray/python3/sage-schemes-hyperelliptic_curves/misc</em> to <em>public/25946</em>
</li>
</ul>
<p>
I propose a solution : simply remove <code>__eq__</code>, <code>__ne__</code> and <code>__hash__</code> here.
</p>
<p>
Then they are all provided by the general setting of projective subschemes, which seems to be a good idea. This will compare ambient spaces and defining ideals, which amount to compare base field and polynomials.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=2726198f766abcf045d53b94c3b0ef9f563de1bf"><span class="icon"></span>2726198</a></td><td><code>Merge branch 'u/embray/python3/sage-schemes-hyperelliptic_curves/misc' of ssh://trac.sagemath.org:22/sage into 8.4.b0</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=78ae9220be1f5279f1766ec78f597bb4634b69e2"><span class="icon"></span>78ae922</a></td><td><code>py3: comparison and hash cleanup for hyperellliptic curves</code>
</td></tr></table>
TicketcremonaTue, 14 Aug 2018 08:20:14 GMT
https://trac.sagemath.org/ticket/25946#comment:19
https://trac.sagemath.org/ticket/25946#comment:19
<p>
That sounds very sensible to me -- I do not know why the person who implemented these special functions thought it was necessary to override those of the parent class.
</p>
<p>
As far as I am concerned this is good to merge assuming that tests all pass.
</p>
TicketchapotonTue, 14 Aug 2018 09:05:24 GMTdescription changed
https://trac.sagemath.org/ticket/25946#comment:20
https://trac.sagemath.org/ticket/25946#comment:20
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/25946?action=diff&version=20">diff</a>)
</li>
</ul>
TicketchapotonTue, 14 Aug 2018 11:42:50 GMT
https://trac.sagemath.org/ticket/25946#comment:21
https://trac.sagemath.org/ticket/25946#comment:21
<p>
failing doctests in hyperelliptic_padic_field.py (sigh)
</p>
TicketembrayTue, 14 Aug 2018 12:16:59 GMT
https://trac.sagemath.org/ticket/25946#comment:22
https://trac.sagemath.org/ticket/25946#comment:22
<p>
This seems like a reasonable approach, but it seems that you exposed a poor assumption that was made somewhere else. I don't know what a <code>sage.rings.padics.padic_ZZ_pX_CR_element.pAdicZZpXCRElement</code> is, or why it isn't hashable. Perhaps it should be?
</p>
<p>
If it definitely shouldn't be hashable (e.g. it is not immutable) then maybe the problem is in <code>MPolynomialIdeal.__richcmp__</code>'s assumption that the generators of an ideal are definitely hashable (and if not, it should do something else to compare gens).
</p>
TicketchapotonTue, 14 Aug 2018 12:22:40 GMT
https://trac.sagemath.org/ticket/25946#comment:23
https://trac.sagemath.org/ticket/25946#comment:23
<p>
There is a <code>_cache_key</code> method on these objects inside <code>src/sage/rings/padics/padic_ZZ_pX_CR_element.pyx</code>
</p>
<p>
But its doc claims that there is no reasonable hash..
</p>
TicketcremonaTue, 14 Aug 2018 12:33:01 GMTcc changed
https://trac.sagemath.org/ticket/25946#comment:24
https://trac.sagemath.org/ticket/25946#comment:24
<ul>
<li><strong>cc</strong>
<em>roed</em> added
</li>
</ul>
<p>
Adding David Roe in CC since he may be able to throw light on the p-adic ring issue. Note that p-adic rings (as with the reals) are not exact, so comparison between elements is not so easy.
</p>
TicketembrayTue, 14 Aug 2018 12:33:59 GMT
https://trac.sagemath.org/ticket/25946#comment:25
https://trac.sagemath.org/ticket/25946#comment:25
<p>
Incidentally, <code>MPolynomialIdeal.__richcmp__</code> was last touched by <a class="closed ticket" href="https://trac.sagemath.org/ticket/23920" title="enhancement: py3: richcmp for ideals of multivariate polynomials (closed: fixed)">#23920</a>. The idea to use sets to compare gens was from Travis: <a class="ext-link" href="https://trac.sagemath.org/ticket/23920#comment:9"><span class="icon"></span>https://trac.sagemath.org/ticket/23920#comment:9</a>
</p>
<p>
Perhaps there should be a try/except around this and attempt direct comparison if comparing by sets raises a <code>TypeError</code>. I'm not sure what the impact is of computing a Groebner basis, or if it can be avoided in some other way as well.
</p>
TicketgitTue, 14 Aug 2018 12:38:41 GMTcommit changed
https://trac.sagemath.org/ticket/25946#comment:26
https://trac.sagemath.org/ticket/25946#comment:26
<ul>
<li><strong>commit</strong>
changed from <em>78ae9220be1f5279f1766ec78f597bb4634b69e2</em> to <em>243172790b441f1feb1f9dfa3cfdff9810f6705d</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=243172790b441f1feb1f9dfa3cfdff9810f6705d"><span class="icon"></span>2431727</a></td><td><code>adding the hash</code>
</td></tr></table>
TicketchapotonTue, 14 Aug 2018 12:39:33 GMT
https://trac.sagemath.org/ticket/25946#comment:27
https://trac.sagemath.org/ticket/25946#comment:27
<p>
adding hash from the existing _cache_key seems to fix the failing doctests.. But is this a correct thing to do ?
</p>
TicketembrayTue, 14 Aug 2018 12:51:18 GMT
https://trac.sagemath.org/ticket/25946#comment:28
https://trac.sagemath.org/ticket/25946#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:27" title="Comment 27">chapoton</a>:
</p>
<blockquote class="citation">
<p>
adding hash from the existing _cache_key seems to fix the failing doctests.. But is this a correct thing to do ?
</p>
</blockquote>
<p>
No, I don't believe so, and the I think the explanation in the <code>_cache_key</code> docs is fairly clear why. Instead: <a class="ext-link" href="https://trac.sagemath.org/ticket/25946#comment:25"><span class="icon"></span>https://trac.sagemath.org/ticket/25946#comment:25</a>
</p>
TicketgitTue, 14 Aug 2018 13:20:10 GMTcommit changed
https://trac.sagemath.org/ticket/25946#comment:29
https://trac.sagemath.org/ticket/25946#comment:29
<ul>
<li><strong>commit</strong>
changed from <em>243172790b441f1feb1f9dfa3cfdff9810f6705d</em> to <em>9f9399ca5bdf787d8cb61f1174f71822b1ea241d</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=d5573ef5de068bdae2165cc9c16f0ced2151219a"><span class="icon"></span>d5573ef</a></td><td><code>py3: add a __hash__ for HyperellipticCurve_generic</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f29c001301afd191bb6a99af2016fdddbbb99926"><span class="icon"></span>f29c001</a></td><td><code>py3: need explicit cast to int for range</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=3771812d49b682d9f8961cff4df7021babda57d9"><span class="icon"></span>3771812</a></td><td><code>py3: comparison and hash cleanup for hyperellliptic curves</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=9f9399ca5bdf787d8cb61f1174f71822b1ea241d"><span class="icon"></span>9f9399c</a></td><td><code>tiny change in the comparison of ideals inside polynomial rings</code>
</td></tr></table>
TicketchapotonTue, 14 Aug 2018 13:22:06 GMT
https://trac.sagemath.org/ticket/25946#comment:30
https://trac.sagemath.org/ticket/25946#comment:30
<p>
ok. So here is my next proposal : when comparing ideals generated by a single polynomial, just compare this generator (no need to wrap by set).
</p>
TicketembrayTue, 14 Aug 2018 13:47:29 GMT
https://trac.sagemath.org/ticket/25946#comment:31
https://trac.sagemath.org/ticket/25946#comment:31
<p>
True. Is that case common enough to merit a special case? Are there cases of ideals of rings over these types of p-adics whose ideal would be generated by more than one polynomials (and hence would still crash on the existing code)?
</p>
<p>
What I'm saying is just, why not:
</p>
<div class="wiki-code"><div class="code"><pre><span class="k">try</span><span class="p">:</span>
same_gens <span class="o">=</span> <span class="nb">set</span><span class="p">(</span><span class="bp">self</span><span class="o">.</span>gens<span class="p">())</span> <span class="o">==</span> <span class="nb">set</span><span class="p">(</span>other<span class="o">.</span>gens<span class="p">())</span>
<span class="k">except</span> <span class="ne">TypeError</span><span class="p">:</span>
same_gens <span class="o">=</span> <span class="bp">False</span>
<span class="k">if</span> same_gens<span class="p">:</span>
<span class="c"># Do whatever we do currently</span>
<span class="k">else</span><span class="p">:</span>
<span class="c"># Do whatever is necessary to compare the gens (construct a GB, etc).</span>
</pre></div></div>
TicketgitTue, 14 Aug 2018 14:07:06 GMTcommit changed
https://trac.sagemath.org/ticket/25946#comment:32
https://trac.sagemath.org/ticket/25946#comment:32
<ul>
<li><strong>commit</strong>
changed from <em>9f9399ca5bdf787d8cb61f1174f71822b1ea241d</em> to <em>ecc4b361b04690708a037dc03c0975a5d312530f</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=ecc4b361b04690708a037dc03c0975a5d312530f"><span class="icon"></span>ecc4b36</a></td><td><code>change again the comparison of ideals</code>
</td></tr></table>
TicketcremonaTue, 14 Aug 2018 14:08:03 GMT
https://trac.sagemath.org/ticket/25946#comment:33
https://trac.sagemath.org/ticket/25946#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:31" title="Comment 31">embray</a>:
</p>
<blockquote class="citation">
<p>
True. Is that case common enough to merit a special case? Are there cases of ideals of rings over these types of p-adics whose ideal would be generated by more than one polynomials (and hence would still crash on the existing code)?
</p>
</blockquote>
<p>
Certainly: in ZZ_p[X] the ideal generated by p and X. But the number of gens will always be small, so why not sort the gens, however many there are?
</p>
<p>
This is an example of a common problem in Sage and other systems. If I define two ideals (or other structures) in different ways and ask whether they are equal then the test may be very expensive. But here we just test whether they have been presented in the same way (or with trivial changes such as changing the order of the generators).
</p>
<blockquote class="citation">
<p>
What I'm saying is just, why not:
</p>
<div class="wiki-code"><div class="code"><pre><span class="k">try</span><span class="p">:</span>
same_gens <span class="o">=</span> <span class="nb">set</span><span class="p">(</span><span class="bp">self</span><span class="o">.</span>gens<span class="p">())</span> <span class="o">==</span> <span class="nb">set</span><span class="p">(</span>other<span class="o">.</span>gens<span class="p">())</span>
<span class="k">except</span> <span class="ne">TypeError</span><span class="p">:</span>
same_gens <span class="o">=</span> <span class="bp">False</span>
<span class="k">if</span> same_gens<span class="p">:</span>
<span class="c"># Do whatever we do currently</span>
<span class="k">else</span><span class="p">:</span>
<span class="c"># Do whatever is necessary to compare the gens (construct a GB, etc).</span>
</pre></div></div></blockquote>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=ecc4b361b04690708a037dc03c0975a5d312530f"><span class="icon"></span>ecc4b36</a></td><td><code>change again the comparison of ideals</code>
</td></tr></table>
TicketchapotonTue, 14 Aug 2018 14:08:55 GMT
https://trac.sagemath.org/ticket/25946#comment:34
https://trac.sagemath.org/ticket/25946#comment:34
<p>
Here is another tentative : first compare without set() then compare with set()
</p>
TicketembrayTue, 14 Aug 2018 14:32:09 GMT
https://trac.sagemath.org/ticket/25946#comment:35
https://trac.sagemath.org/ticket/25946#comment:35
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:34" title="Comment 34">chapoton</a>:
</p>
<blockquote class="citation">
<p>
Here is another tentative : first compare without set() then compare with set()
</p>
</blockquote>
<p>
That could still crash. I don't know the math well enough to construct an interestingexample case, but say you have two copies of the same ideal, but their generators just happen to be in a different order, as in:
</p>
<pre class="wiki">sage: R.<x,y> = QQ[]
sage: R.ideal(x, y).gens()
[x, y]
sage: R.ideal(y, x).gens()
</pre><p>
This is the sort of case, as John wrote, that the <code>set()</code> call is intended for in the first place. But if the elements <code>x</code> and <code>y</code> are not hashable, then the first test will still fail, and then the second test will be evaluated and will crash.
</p>
<p>
IMO this comparison of gens() is just a special case shortcut which is fine as-is, but it should account for the possibility that some elements just aren't hashable, that's all.
</p>
TicketcremonaTue, 14 Aug 2018 14:45:10 GMT
https://trac.sagemath.org/ticket/25946#comment:36
https://trac.sagemath.org/ticket/25946#comment:36
<p>
Does hashable imply not sortable? As I said, the lists of gens will be short normally, so something like testing sorted(s_gens)==sorted(o_gens) might work when the set() constructor does not? There will still be silly trivial cases such as first giving {x,y} as gens then {x,x,y} and for sure someone will then complain. But even as it is, we are not returning equality of the ideals (x,y) and (x+y,y). We are never going to catch all trivial cases (and different users' idea of a trivial case will differ) so just comparing the list of gens with no processing at all is safest and most easily explained.
</p>
TicketgitTue, 14 Aug 2018 14:47:33 GMTcommit changed
https://trac.sagemath.org/ticket/25946#comment:37
https://trac.sagemath.org/ticket/25946#comment:37
<ul>
<li><strong>commit</strong>
changed from <em>ecc4b361b04690708a037dc03c0975a5d312530f</em> to <em>d47fbb3f350c6e5ce4d2fdc809a3849699bebdd0</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=d47fbb3f350c6e5ce4d2fdc809a3849699bebdd0"><span class="icon"></span>d47fbb3</a></td><td><code>wrap with try</code>
</td></tr></table>
TicketchapotonTue, 14 Aug 2018 14:48:47 GMT
https://trac.sagemath.org/ticket/25946#comment:38
https://trac.sagemath.org/ticket/25946#comment:38
<p>
ok, I have wrapped with "try / except".
</p>
TicketembrayTue, 14 Aug 2018 14:55:48 GMT
https://trac.sagemath.org/ticket/25946#comment:39
https://trac.sagemath.org/ticket/25946#comment:39
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:36" title="Comment 36">cremona</a>:
</p>
<blockquote class="citation">
<p>
Does hashable imply not sortable? As I said, the lists of gens will be short normally, so something like testing sorted(s_gens)==sorted(o_gens) might work when the set() constructor does not? There will still be silly trivial cases such as first giving {x,y} as gens then {x,x,y} and for sure someone will then complain. But even as it is, we are not returning equality of the ideals (x,y) and (x+y,y). We are never going to catch all trivial cases (and different users' idea of a trivial case will differ) so just comparing the list of gens with no processing at all is safest and most easily explained.
</p>
</blockquote>
<p>
I don't think sorting really helps here, because as I found out over in <a class="closed ticket" href="https://trac.sagemath.org/ticket/25948" title="defect: py3: a few more miscellaneous dict iterator (dict.keys, dict.values) fixes (closed: fixed)">#25948</a>, some elements can be "sorted" but the ordering is meaningless and unpredictable.
</p>
TicketembrayTue, 14 Aug 2018 14:56:54 GMT
https://trac.sagemath.org/ticket/25946#comment:40
https://trac.sagemath.org/ticket/25946#comment:40
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:37" title="Comment 37">git</a>:
</p>
<blockquote class="citation">
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=d47fbb3f350c6e5ce4d2fdc809a3849699bebdd0"><span class="icon"></span>d47fbb3</a></td><td><code>wrap with try</code>
</td></tr></table>
</blockquote>
<p>
You could still get rid of <code>(s_gens == o_gens)</code>. I don't think it's that useful.
</p>
TicketchapotonTue, 14 Aug 2018 14:59:47 GMT
https://trac.sagemath.org/ticket/25946#comment:41
https://trac.sagemath.org/ticket/25946#comment:41
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:40" title="Comment 40">embray</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25946#comment:37" title="Comment 37">git</a>:
</p>
<blockquote class="citation">
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=d47fbb3f350c6e5ce4d2fdc809a3849699bebdd0"><span class="icon"></span>d47fbb3</a></td><td><code>wrap with try</code>
</td></tr></table>
</blockquote>
<p>
You could still get rid of <code>(s_gens == o_gens)</code>. I don't think it's that useful.
</p>
</blockquote>
<p>
Yes it is. It will handle the precise case that we are dealing with in this ticket, hyperelliptic curves over p-adics.
</p>
TicketembrayTue, 14 Aug 2018 15:06:26 GMT
https://trac.sagemath.org/ticket/25946#comment:42
https://trac.sagemath.org/ticket/25946#comment:42
<p>
Ok, but I think as written you don't need the <code>same_gens</code> variable. I only wrote it that way in my pseudo-code because I wasn't directly looking at the real code at the time, and didn't consider using a try/except/else. I don't think we need to worry about <code>rich_to_bool</code> raising a <code>TypeError</code>.
</p>
TicketgitTue, 14 Aug 2018 15:13:11 GMTcommit changed
https://trac.sagemath.org/ticket/25946#comment:43
https://trac.sagemath.org/ticket/25946#comment:43
<ul>
<li><strong>commit</strong>
changed from <em>d47fbb3f350c6e5ce4d2fdc809a3849699bebdd0</em> to <em>b129fc96811a9db8c454a395180e6ca2a8a1b5fb</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=b129fc96811a9db8c454a395180e6ca2a8a1b5fb"><span class="icon"></span>b129fc9</a></td><td><code>fix</code>
</td></tr></table>
TicketchapotonTue, 14 Aug 2018 15:13:25 GMT
https://trac.sagemath.org/ticket/25946#comment:44
https://trac.sagemath.org/ticket/25946#comment:44
<p>
like that ?
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=b129fc96811a9db8c454a395180e6ca2a8a1b5fb"><span class="icon"></span>b129fc9</a></td><td><code>fix</code>
</td></tr></table>
TicketembrayTue, 14 Aug 2018 15:22:15 GMT
https://trac.sagemath.org/ticket/25946#comment:45
https://trac.sagemath.org/ticket/25946#comment:45
<p>
Yeah, though it would be nice to squash all these commits before we're done here.
</p>
TicketgitTue, 14 Aug 2018 15:25:18 GMTcommit changed
https://trac.sagemath.org/ticket/25946#comment:46
https://trac.sagemath.org/ticket/25946#comment:46
<ul>
<li><strong>commit</strong>
changed from <em>b129fc96811a9db8c454a395180e6ca2a8a1b5fb</em> to <em>f8687f7d8800aede97e36d4da30ae79da895b5f7</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f8687f7d8800aede97e36d4da30ae79da895b5f7"><span class="icon"></span>f8687f7</a></td><td><code>py3: comparison cleanup for hyperelliptic curves</code>
</td></tr></table>
TicketchapotonTue, 14 Aug 2018 15:25:28 GMT
https://trac.sagemath.org/ticket/25946#comment:47
https://trac.sagemath.org/ticket/25946#comment:47
<p>
done
</p>
TicketembrayTue, 14 Aug 2018 15:32:35 GMT
https://trac.sagemath.org/ticket/25946#comment:48
https://trac.sagemath.org/ticket/25946#comment:48
<p>
Thanks! Well, I'm happy, so you can set positive review if you want, or wait and see if anyone smarter than me wants to weigh on, though it seems like this approach meshes with what the other experts have said so far.
</p>
TicketcremonaTue, 14 Aug 2018 15:38:38 GMT
https://trac.sagemath.org/ticket/25946#comment:49
https://trac.sagemath.org/ticket/25946#comment:49
<p>
I am happy (and making no claims to be cleverer than anyone else ;))
</p>
<p>
I would never have thought of using rich_to_bool() though. Not clever enough!
</p>
TicketroedTue, 14 Aug 2018 16:07:29 GMTstatus, reviewer changed
https://trac.sagemath.org/ticket/25946#comment:50
https://trac.sagemath.org/ticket/25946#comment:50
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Travis Scrimshaw</em> to <em>Travis Scrimshaw, David Roe</em>
</li>
</ul>
<p>
Seems good to me too!
</p>
TicketvbraunFri, 17 Aug 2018 21:14:06 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/25946#comment:51
https://trac.sagemath.org/ticket/25946#comment:51
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>public/25946</em> to <em>f8687f7d8800aede97e36d4da30ae79da895b5f7</em>
</li>
</ul>
Ticket