Sage: Ticket #7160: comparison with quadratic number field elements
https://trac.sagemath.org/ticket/7160
<pre class="wiki">sage: I^2
-1
sage: bool(I^2 < 0)
True
sage: bool(I^2 > 0)
True
</pre><p>
Apply:
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/7160/trac_7160-doctest.take2.patch" title="Attachment 'trac_7160-doctest.take2.patch' in Ticket #7160">trac_7160-doctest.take2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/7160/trac_7160-doctest.take2.patch" title="Download"></a>
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/7160
Trac 1.1.6mhansenThu, 08 Oct 2009 18:41:58 GMTstatus changed
https://trac.sagemath.org/ticket/7160#comment:1
https://trac.sagemath.org/ticket/7160#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketmhansenThu, 08 Oct 2009 19:23:54 GMT
https://trac.sagemath.org/ticket/7160#comment:2
https://trac.sagemath.org/ticket/7160#comment:2
<p>
This is basically the same as <a class="closed ticket" href="https://trac.sagemath.org/ticket/6132" title="defect: [with patch, needs work] cmp for number field elements (closed: duplicate)">#6132</a>.
</p>
TicketcremonaThu, 29 Oct 2009 21:30:58 GMTstatus changed
https://trac.sagemath.org/ticket/7160#comment:3
https://trac.sagemath.org/ticket/7160#comment:3
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I think this is confusing: you have an ordering of elements of a real or imaginary quadratic field which is based on some kind of lexicographic ordering on the representation of the elements rather than anything mathematical. Presumably this is so lists, etc, can be sorted in a pythonic way. But I find this bizarre:
</p>
<pre class="wiki">sage: K.<i> = QuadraticField(-1)
sage: i>0
True
</pre><p>
and this raises a strange error:
</p>
<pre class="wiki">sage: K.<a> = NumberField(x^2+x-10)
sage: a < -a
---------------------------------------------------------------------------
SystemError Traceback (most recent call last)
/home/john/.sage/temp/ubuntu/790/_home_john__sage_init_sage_0.py in <module>()
SystemError: error return without exception set
sage: Type(K)
---------------------------------------------------------------------------
NameError Traceback (most recent call last)
/home/john/.sage/temp/ubuntu/790/_home_john__sage_init_sage_0.py in <module>()
NameError: name 'Type' is not defined
sage: type(K)
<class 'sage.rings.number_field.number_field.NumberField_quadratic'>
</pre>
TicketkcrismanMon, 07 Feb 2011 15:43:47 GMTupstream set
https://trac.sagemath.org/ticket/7160#comment:4
https://trac.sagemath.org/ticket/7160#comment:4
<ul>
<li><strong>upstream</strong>
set to <em>N/A</em>
</li>
</ul>
<p>
Apparently related to <a class="closed ticket" href="https://trac.sagemath.org/ticket/6132" title="defect: [with patch, needs work] cmp for number field elements (closed: duplicate)">#6132</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10064" title="defect: -1 in expression "is_positive". (closed: fixed)">#10064</a>, see <a class="ext-link" href="http://groups.google.com/group/sage-support/browse_thread/thread/28bbd04a78dadb57/01168722573ff736"><span class="icon"></span>this sage-devel discussion</a>.
</p>
TicketmhansenMon, 19 Dec 2011 10:32:31 GMTstatus changed
https://trac.sagemath.org/ticket/7160#comment:5
https://trac.sagemath.org/ticket/7160#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I've put up a new patch which is minimally invasive and fixes the issue.
</p>
TicketkcrismanMon, 19 Dec 2011 15:02:45 GMT
https://trac.sagemath.org/ticket/7160#comment:6
https://trac.sagemath.org/ticket/7160#comment:6
<p>
Does this result in any interesting speed regressions when doing things with I?
</p>
TicketwasTue, 20 Dec 2011 06:47:51 GMT
https://trac.sagemath.org/ticket/7160#comment:7
https://trac.sagemath.org/ticket/7160#comment:7
<ol><li>Typo: " comparision" in sage/rings/number_field/number_field_element_quadratic.pyx.
</li></ol><ol start="2"><li>This code raises a big red flag for me:
<pre class="wiki"> 1704 from sage.rings.all import RR
1705 r = RR(right)
1706 l = RR(embedding(left))
1707 return cmp(l, r)
</pre></li></ol><p>
My concern is that you're comparing to double (=53 bits) precision, which is totally arbitrary. What if there is an embedding and cmp(left, right) requires 54 bits of precision to sort out? I could probably with some work construct a nasty example of this, where the above code just gives totally the wrong answer. I guess we need to (1) compute to precision of the embedding, and (2) if cmp(l,r)==0, then check something more?
</p>
<ol start="3"><li>Where is "self._element_class" actually used? I guess by the coercion model, but it's hard to see how from this patch, exactly.
</li></ol><ol start="4"><li>I feel like this is too much code that gets around a fundamental problem or bug in number field elements in the case of the reported problem, but leaves the underlying bug unfixed. Note that *before* applying your patch:
<pre class="wiki">sage: bool(I^2 < 0)
True
sage: (I^2).pyobject() < 0
False
</pre></li></ol><p>
It seems to me that the output of both lines above should be the same, right, but I bet pynac is evaluating the first comparison and not even going to the pyobject level. Similarly:
</p>
<pre class="wiki">sage: bool(I^2 < SR(0))
True
sage: (I^2).pyobject() < SR(0).pyobject()
False
</pre><p>
This is because in Sage we currently have:
</p>
<pre class="wiki">sage: K.<I> = QuadraticField(-1)
sage: I^2 < 0
False
sage: I^2
-1
</pre><p>
This is, as Cremona pointed out, due to the arbitrary lexicographic ordering rather than using the one we get on the intersection of K and R inside embedding(K).
</p>
<p>
So... it seems to me that the real problem is that the arbitrary ordering is lame. What you've done is implemented a natural fix in one very, very special case. Maybe that's the intention, and maybe you had something more general before? I don't know, since you changed the patch.
</p>
<p>
I'm guessing your more general patch changed comparison for all elements to:
</p>
<blockquote>
<p>
(1) check if there is an embedding of the parent(s), and (2) if so, use that to induce an embedding on *the* real subfield of the field(s). That would be natural.
</p>
</blockquote>
<p>
So all of the above, is just me understanding why you are doing what you're doing. It might make sense to add something like this to the documentation.
</p>
TicketwasTue, 20 Dec 2011 07:22:26 GMT
https://trac.sagemath.org/ticket/7160#comment:8
https://trac.sagemath.org/ticket/7160#comment:8
<p>
Note: With sage-4.8.alpha5 and this patch, all tests in devel/sage/sage pass.
</p>
TicketdavidloefflerTue, 20 Dec 2011 09:40:24 GMT
https://trac.sagemath.org/ticket/7160#comment:9
https://trac.sagemath.org/ticket/7160#comment:9
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7160#comment:7" title="Comment 7">was</a>:
</p>
<blockquote class="citation">
<ol start="2"><li>This code raises a big red flag for me:
</li></ol></blockquote>
<pre class="wiki"> 1704 from sage.rings.all import RR
1705 r = RR(right)
1706 l = RR(embedding(left))
1707 return cmp(l, r)
</pre><blockquote class="citation">
<p>
My concern is that you're comparing to double (=53 bits) precision, which is totally arbitrary. What if there is an embedding and cmp(left, right) requires 54 bits of precision to sort out? I could probably with some work construct a nasty example of this, where the above code just gives totally the wrong answer. I guess we need to (1) compute to precision of the embedding, and (2) if cmp(l,r)==0, then check something more?
</p>
</blockquote>
<p>
Isn't this what we have the QQbar class for?
</p>
TicketmhansenTue, 20 Dec 2011 10:09:51 GMTattachment set
https://trac.sagemath.org/ticket/7160
https://trac.sagemath.org/ticket/7160
<ul>
<li><strong>attachment</strong>
set to <em>trac_7160.patch</em>
</li>
</ul>
TicketmhansenTue, 20 Dec 2011 10:10:47 GMT
https://trac.sagemath.org/ticket/7160#comment:10
https://trac.sagemath.org/ticket/7160#comment:10
<p>
I've added a new patch which should be better. It turns out just using the embedding directly should be fine.
</p>
TicketmhansenTue, 20 Dec 2011 12:32:49 GMTkeywords set
https://trac.sagemath.org/ticket/7160#comment:11
https://trac.sagemath.org/ticket/7160#comment:11
<ul>
<li><strong>keywords</strong>
<em>sd35</em> added
</li>
</ul>
TicketburcinTue, 10 Jan 2012 01:07:58 GMTattachment set
https://trac.sagemath.org/ticket/7160
https://trac.sagemath.org/ticket/7160
<ul>
<li><strong>attachment</strong>
set to <em>trac_7160-Q_to_quadratic_field_element.patch</em>
</li>
</ul>
TicketburcinTue, 10 Jan 2012 05:41:36 GMTmilestone, author changed
https://trac.sagemath.org/ticket/7160#comment:12
https://trac.sagemath.org/ticket/7160#comment:12
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.8</em> to <em>sage-5.0</em>
</li>
<li><strong>author</strong>
changed from <em>Mike Hansen</em> to <em>Mike Hansen, Burcin Erocal</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7160#comment:7" title="Comment 7">was</a>:
</p>
<blockquote class="citation">
<ol start="4"><li>I feel like this is too much code that gets around a fundamental problem or bug in number field elements in the case of the reported problem, but leaves the underlying bug unfixed. Note that *before* applying your patch:
</li></ol></blockquote>
<pre class="wiki">sage: bool(I^2 < 0)
True
sage: (I^2).pyobject() < 0
False
</pre><blockquote class="citation">
<p>
It seems to me that the output of both lines above should be the same, right, but I bet pynac is evaluating the first comparison and not even going to the pyobject level.
</p>
</blockquote>
<p>
The first comparison is done in the <code>test_relation()</code> method of <code>sage.symbolic.expression.Expression</code>. This converts both sides of the relation to <code>CIF</code> and performs the comparison there.
</p>
<blockquote class="citation">
<p>
Similarly:
</p>
<pre class="wiki">sage: bool(I^2 < SR(0))
True
sage: (I^2).pyobject() < SR(0).pyobject()
False
</pre><p>
This is because in Sage we currently have:
</p>
<pre class="wiki">sage: K.<I> = QuadraticField(-1)
sage: I^2 < 0
False
sage: I^2
-1
</pre><p>
This is, as Cremona pointed out, due to the arbitrary lexicographic ordering rather than using the one we get on the intersection of K and R inside embedding(K).
</p>
<p>
So... it seems to me that the real problem is that the arbitrary ordering is lame. What you've done is implemented a natural fix in one very, very special case. Maybe that's the intention, and maybe you had something more general before? I don't know, since you changed the patch.
</p>
</blockquote>
<p>
Note that this lame ordering is causing problems all over symbolics since <code>is_positive()</code> checks fail. See <a class="closed ticket" href="https://trac.sagemath.org/ticket/10849" title="defect: behaviour of gamma strangely sensitive (closed: fixed)">#10849</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/10062" title="defect: With imag, sqrt and subs I get 0==1 (closed: fixed)">#10062</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/10064" title="defect: -1 in expression "is_positive". (closed: fixed)">#10064</a> for examples.
</p>
<blockquote class="citation">
<p>
I'm guessing your more general patch changed comparison for all elements to:
</p>
<blockquote>
<p>
(1) check if there is an embedding of the parent(s), and (2) if so, use that to induce an embedding on *the* real subfield of the field(s). That would be natural.
</p>
</blockquote>
</blockquote>
<p>
I attached a new patch at <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/7160/trac_7160-quadratic_number_field_element_comparison.patch" title="Attachment 'trac_7160-quadratic_number_field_element_comparison.patch' in Ticket #7160">attachment:trac_7160-quadratic_number_field_element_comparison.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/7160/trac_7160-quadratic_number_field_element_comparison.patch" title="Download"></a>, based on Mike's last patch <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/7160/trac_7160.patch" title="Attachment 'trac_7160.patch' in Ticket #7160">attachment:trac_7160.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/7160/trac_7160.patch" title="Download"></a>. This changes the comparison function of quadratic number field elements to always use the embedding into <code>CC</code>.
</p>
<p>
Timings with the patch:
</p>
<pre class="wiki">sage: K.<sqrt2> = QuadraticField(2)
sage: t = K.random_element(); t
-2*sqrt2 - 35
sage: u = K.random_element(); u
-1
sage: %timeit res = cmp(t,u)
625 loops, best of 3: 659 µs per loop
sage: u = K.random_element(); u
-2*sqrt2 - 1
sage: %timeit res = cmp(t,u)
625 loops, best of 3: 807 µs per loop
</pre><p>
Without the patch:
</p>
<pre class="wiki">sage: K.<sqrt2> = QuadraticField(2)
sage: t = -2*sqrt2 - 35
sage: u = K(-1)
sage: %timeit res = cmp(t,u)
625 loops, best of 3: 419 ns per loop
sage: u = -2*sqrt2 - 1
sage: %timeit res = cmp(t,u)
625 loops, best of 3: 419 ns per loop
</pre><p>
So there is a significant slow down.
</p>
<p>
There are two failing doctests I need help with:
</p>
<pre class="wiki">sage -t devel/sage/sage/schemes/elliptic_curves/ell_curve_isogeny.py
**********************************************************************
File "/home/burcin/sage/sage-5.0.prealpha0/devel/sage-main/sage/schemes/elliptic_curves/ell_curve_isogeny.py", line 4361:
sage: _[0].rational_maps()
Expected:
(((-4/25*i - 3/25)*x^5 + (-4/5*i + 2/5)*x^3 + x)/(x^4 + (-4/5*i + 2/5)*x^2 + (-4/25*i - 3/25)),
((2/125*i - 11/125)*x^6*y + (64/125*i + 23/125)*x^4*y + (162/125*i - 141/125)*x^2*y + (-4/25*i - 3/25)*y)/(x^6 + (-6/5*i + 3/5)*x^4 + (-12/25*i - 9/25)*x^2 + (2/125*i - 11/125)))
Got:
(((4/25*i + 3/25)*x^5 + (4/5*i - 2/5)*x^3 - x)/(x^4 + (-4/5*i + 2/5)*x^2 + (-4/25*i - 3/25)), ((11/125*i + 2/125)*x^6*y + (-23/125*i + 64/125)*x^4*y + (141/125*i + 162/125)*x^2*y + (3/25*i - 4/25)*y)/(x^6 + (-6/5*i + 3/5)*x^4 + (-12/25*i - 9/25)*x^2 + (2/125*i - 11/125)))
**********************************************************************
File "/home/burcin/sage/sage-5.0.prealpha0/devel/sage-main/sage/schemes/elliptic_curves/ell_curve_isogeny.py", line 4622:
sage: isogenies_13_0(E)[0].rational_maps()
Expected:
(((-4/169*r - 11/169)*x^13 + (-128/13*r - 456/13)*x^10 + (-1224/13*r - 2664/13)*x^7 + (-2208/13*r + 5472/13)*x^4 + (1152/13*r - 8064/13)*x)/(x^12 + (4*r - 36)*x^9 + (-1080/13*r + 3816/13)*x^6 + (2112/13*r + 5184/13)*x^3 + (17280/169*r - 1152/169)), ((18/2197*r - 35/2197)*x^18*y + (-23142/2197*r + 35478/2197)*x^15*y + (-1127520/2197*r + 1559664/2197)*x^12*y + (87744/2197*r + 5992704/2197)*x^9*y + (-6625152/2197*r + 9085824/2197)*x^6*y + (28919808/2197*r - 2239488/2197)*x^3*y + (-1990656/2197*r + 3870720/2197)*y)/(x^18 + (6*r - 54)*x^15 + (-3024/13*r + 11808/13)*x^12 + (31296/13*r - 51840/13)*x^9 + (-487296/169*r - 2070144/169)*x^6 + (-940032/169*r - 248832/169)*x^3 + (-1990656/2197*r + 3870720/2197)))
Got:
(((7/338*r + 23/338)*x^13 + (-164/13*r - 420/13)*x^10 + (720/13*r + 3168/13)*x^7 + (3840/13*r - 576/13)*x^4 + (4608/13*r + 2304/13)*x)/(x^12 + (4*r + 36)*x^9 + (1080/13*r + 3816/13)*x^6 + (2112/13*r - 5184/13)*x^3 + (-17280/169*r - 1152/169)), ((18/2197*r + 35/2197)*x^18*y + (23142/2197*r + 35478/2197)*x^15*y + (-1127520/2197*r - 1559664/2197)*x^12*y + (-87744/2197*r + 5992704/2197)*x^9*y + (-6625152/2197*r - 9085824/2197)*x^6*y + (-28919808/2197*r - 2239488/2197)*x^3*y + (-1990656/2197*r - 3870720/2197)*y)/(x^18 + (6*r + 54)*x^15 + (3024/13*r + 11808/13)*x^12 + (31296/13*r + 51840/13)*x^9 + (487296/169*r - 2070144/169)*x^6 + (-940032/169*r + 248832/169)*x^3 + (1990656/2197*r + 3870720/2197)))
</pre>
TicketdavidloefflerThu, 29 Mar 2012 17:21:36 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/7160#comment:13
https://trac.sagemath.org/ticket/7160#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>needs rebase</em>
</li>
</ul>
<p>
This patch doesn't apply to 5.0.beta11 -- see patchbot logs.
</p>
TicketmhansenThu, 29 Mar 2012 20:23:37 GMTattachment set
https://trac.sagemath.org/ticket/7160
https://trac.sagemath.org/ticket/7160
<ul>
<li><strong>attachment</strong>
set to <em>trac_7160-quadratic_number_field_element_comparison.patch</em>
</li>
</ul>
<p>
apply only this patch
</p>
TicketmhansenThu, 29 Mar 2012 20:23:56 GMT
https://trac.sagemath.org/ticket/7160#comment:14
https://trac.sagemath.org/ticket/7160#comment:14
<p>
I rebased the patch to beta11.
</p>
TicketmjoSun, 15 Apr 2012 02:34:45 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/7160#comment:15
https://trac.sagemath.org/ticket/7160#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>needs rebase</em> deleted
</li>
</ul>
<p>
This applies cleanly on beta13, I believe it needs review again?
</p>
TicketdsmFri, 25 May 2012 04:44:53 GMT
https://trac.sagemath.org/ticket/7160#comment:16
https://trac.sagemath.org/ticket/7160#comment:16
<p>
This ticket applies against cleanly against 5.1.beta0 but still has the two doctest failures. This patch would get rid of several other problems -- <a class="closed ticket" href="https://trac.sagemath.org/ticket/10064" title="defect: -1 in expression "is_positive". (closed: fixed)">#10064</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10849" title="defect: behaviour of gamma strangely sensitive (closed: fixed)">#10849</a> -- so it'd be nice to get in.
</p>
TicketmjoTue, 29 May 2012 19:32:12 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/7160#comment:17
https://trac.sagemath.org/ticket/7160#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>doctest failures</em>
</li>
</ul>
TicketvdelecroixMon, 09 Jul 2012 07:45:53 GMT
https://trac.sagemath.org/ticket/7160#comment:18
https://trac.sagemath.org/ticket/7160#comment:18
<p>
Hi,
</p>
<p>
I made a sort of duplicate with <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a>. My aim was to use the natural order of RR in the case of real embedding. For quadratic field it is quite easy and fast. In the same patch, I aim to implement other functions related to the real embedding (is_positive, floor, ceil, abs, ...).
</p>
<p>
On the other hand, the difference of timings with my patch have an order of magnitude of x10 and not of x1000.
</p>
<p>
Should I close my ticket ? What to do with my patch ?
</p>
TicketvdelecroixMon, 09 Jul 2012 07:52:48 GMTcc set
https://trac.sagemath.org/ticket/7160#comment:19
https://trac.sagemath.org/ticket/7160#comment:19
<ul>
<li><strong>cc</strong>
<em>vdelecroix</em> added
</li>
</ul>
TicketburcinTue, 10 Jul 2012 15:57:40 GMT
https://trac.sagemath.org/ticket/7160#comment:20
https://trac.sagemath.org/ticket/7160#comment:20
<p>
Hi Vincent,
</p>
<p>
thanks a lot for taking a look at this long standing problem.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7160#comment:18" title="Comment 18">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
I made a sort of duplicate with <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a>. My aim was to use the natural order of RR in the case of real embedding. For quadratic field it is quite easy and fast. In the same patch, I aim to implement other functions related to the real embedding (is_positive, floor, ceil, abs, ...).
</p>
</blockquote>
<p>
Fixing the comparison of quadratic number fields elements and implementing <code>floor</code>, <code>ceil</code>, etc. should be in separate patches.
</p>
<blockquote class="citation">
<p>
On the other hand, the difference of timings with my patch have an order of magnitude of x10 and not of x1000.
</p>
<p>
Should I close my ticket ? What to do with my patch ?
</p>
</blockquote>
<p>
Since your patch performs much better, we can move ahead using that. I will try to review your patch on <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a>.
</p>
<p>
Once the patches in this ticket are rebased on your patch, we can close this as a duplicate.
</p>
TicketvdelecroixWed, 15 May 2013 17:13:49 GMT
https://trac.sagemath.org/ticket/7160#comment:21
https://trac.sagemath.org/ticket/7160#comment:21
<p>
Now <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a> is in positive review (and performances much better than what I expected at the begining)! Should we add a doctest related to the example in the description of the ticket or do we simply close it as a duplicate ?
</p>
TicketkcrismanWed, 15 May 2013 18:08:58 GMT
https://trac.sagemath.org/ticket/7160#comment:22
https://trac.sagemath.org/ticket/7160#comment:22
<blockquote class="citation">
<p>
Now <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a> is in positive review (and performances much better than what I expected at the begining)! Should we add a doctest related to the example in the description of the ticket or do we simply close it as a duplicate ?
</p>
</blockquote>
<p>
If there are nearly identical examples to these in <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a>, then I think that's okay; if there is nothing like this there, we should add a trivial patch. to check it's improved. I'm not going to look through that whole patch to find ones like this but presumably you know whether there is one like that, as the author :)
</p>
TicketvdelecroixWed, 15 May 2013 20:25:48 GMTattachment set
https://trac.sagemath.org/ticket/7160
https://trac.sagemath.org/ticket/7160
<ul>
<li><strong>attachment</strong>
set to <em>trac_7160-doctest.patch</em>
</li>
</ul>
TicketvdelecroixWed, 15 May 2013 20:36:59 GMTstatus, description changed
https://trac.sagemath.org/ticket/7160#comment:23
https://trac.sagemath.org/ticket/7160#comment:23
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/7160?action=diff&version=23">diff</a>)
</li>
</ul>
<p>
There are no example which deals with the symbolic ring... that's why I suggested to add some doctest here. See the proposition in the patch. Nevertheless, I found stupid the following behavior (or at least confusing)
</p>
<pre class="wiki">sage: I**2 == -1
-1 == -1
sage: (I+4)**4 > 0
-4 > 0
</pre><p>
apply trac_7160-doctest.patch
</p>
TicketvdelecroixWed, 15 May 2013 20:40:56 GMTdependencies set
https://trac.sagemath.org/ticket/7160#comment:24
https://trac.sagemath.org/ticket/7160#comment:24
<ul>
<li><strong>dependencies</strong>
set to <em>#13213</em>
</li>
</ul>
TicketvdelecroixFri, 31 May 2013 22:15:44 GMTsummary changed
https://trac.sagemath.org/ticket/7160#comment:25
https://trac.sagemath.org/ticket/7160#comment:25
<ul>
<li><strong>summary</strong>
changed from <em>comparison with quadratic number field elements needs work</em> to <em>comparison with quadratic number field elements</em>
</li>
</ul>
<p>
Now <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a> is positive review. I guess we may close that ticket ?
</p>
TicketburcinMon, 03 Jun 2013 13:52:04 GMTattachment set
https://trac.sagemath.org/ticket/7160
https://trac.sagemath.org/ticket/7160
<ul>
<li><strong>attachment</strong>
set to <em>trac_7160-doctest.take2.patch</em>
</li>
</ul>
<p>
fix commit message, and sphinx trac link
</p>
TicketburcinMon, 03 Jun 2013 13:56:25 GMTstatus, description, author changed; reviewer set
https://trac.sagemath.org/ticket/7160#comment:26
https://trac.sagemath.org/ticket/7160#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Burcin Erocal</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/7160?action=diff&version=26">diff</a>)
</li>
<li><strong>author</strong>
changed from <em>Mike Hansen, Burcin Erocal</em> to <em>Vincent Delecroix</em>
</li>
</ul>
<p>
I attached a new patch which fixes the commit message and missing <code>:</code> before the trac link in the documentation. This is good to go.
</p>
<p>
Thanks!
</p>
TicketjdemeyerMon, 03 Jun 2013 15:40:11 GMTmilestone changed; work_issues deleted
https://trac.sagemath.org/ticket/7160#comment:27
https://trac.sagemath.org/ticket/7160#comment:27
<ul>
<li><strong>work_issues</strong>
<em>doctest failures</em> deleted
</li>
<li><strong>milestone</strong>
changed from <em>sage-5.10</em> to <em>sage-5.11</em>
</li>
</ul>
TicketjdemeyerThu, 06 Jun 2013 12:31:18 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/7160#comment:28
https://trac.sagemath.org/ticket/7160#comment:28
<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>merged</strong>
set to <em>sage-5.11.beta0</em>
</li>
</ul>
Ticket