Sage: Ticket #12121: floor/ceil can be very slow at integral values
https://trac.sagemath.org/ticket/12121
<p>
Reported (in slightly different form) on <a class="ext-link" href="http://ask.sagemath.org/question/964/lenlist-ceillog42-bugs"><span class="icon"></span>ask.sagemath.org</a>:
</p>
<pre class="wiki">sage: %timeit floor(log(3)/log(2))
625 loops, best of 3: 586 µs per loop
sage: %timeit floor(log(4)/log(2))
5 loops, best of 3: 3.79 s per loop
</pre><p>
This happens because ceil and floor first try to increase the precision of a coercion of the input argument to a <code>RealInterval</code> by 100 bits from 53 to 20000 before finally trying a full_simplify, which succeeds. The <code>RealInterval</code> rounds all fail because the interval is always of the form (2 - epsilon, 2 + epsilon) and endpoints have different ceilings.
</p>
<p>
With the branch applied <code>math.floor</code> and <code>numpy.floor</code> are used directly
</p>
<pre class="wiki">sage: floor(1.2r)
1.0
sage: type(_)
<type 'float'>
</pre><p>
which is distinct from the current Sage behavior
</p>
<pre class="wiki">sage: floor(1.2r)
1
sage: type(_)
<type 'sage.rings.integer.Integer'>
</pre>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/12121
Trac 1.1.6dsmTue, 06 Dec 2011 15:55:19 GMTdescription changed
https://trac.sagemath.org/ticket/12121#comment:1
https://trac.sagemath.org/ticket/12121#comment:1
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/12121?action=diff&version=1">diff</a>)
</li>
</ul>
TicketdsmTue, 06 Dec 2011 15:55:32 GMTdescription changed
https://trac.sagemath.org/ticket/12121#comment:2
https://trac.sagemath.org/ticket/12121#comment:2
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/12121?action=diff&version=2">diff</a>)
</li>
</ul>
TicketdsmSat, 03 Mar 2012 16:43:37 GMT
https://trac.sagemath.org/ticket/12121#comment:3
https://trac.sagemath.org/ticket/12121#comment:3
<p>
Note to self (and others): we can use is_int to decide when we should test for equality. Test (once) the first time there's only one integer in the interval. If you have equality there, you're done. If not, you never will (or won't be able to prove it, anyway) and can carry on.
</p>
TicketkiniFri, 22 Jun 2012 19:10:45 GMT
https://trac.sagemath.org/ticket/12121#comment:4
https://trac.sagemath.org/ticket/12121#comment:4
<p>
Apparently <a class="ext-link" href="http://thread.gmane.org/gmane.comp.mathematics.sage.devel/57910/"><span class="icon"></span>is_int is deprecated</a>.
</p>
TicketjdemeyerTue, 13 Aug 2013 15:35:53 GMTmilestone changed
https://trac.sagemath.org/ticket/12121#comment:5
https://trac.sagemath.org/ticket/12121#comment:5
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.11</em> to <em>sage-5.12</em>
</li>
</ul>
Ticketvbraun_spamThu, 30 Jan 2014 21:20:52 GMTmilestone changed
https://trac.sagemath.org/ticket/12121#comment:6
https://trac.sagemath.org/ticket/12121#comment:6
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.1</em> to <em>sage-6.2</em>
</li>
</ul>
Ticketvbraun_spamTue, 06 May 2014 15:20:58 GMTmilestone changed
https://trac.sagemath.org/ticket/12121#comment:7
https://trac.sagemath.org/ticket/12121#comment:7
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.2</em> to <em>sage-6.3</em>
</li>
</ul>
Ticketvbraun_spamSun, 10 Aug 2014 16:51:03 GMTmilestone changed
https://trac.sagemath.org/ticket/12121#comment:8
https://trac.sagemath.org/ticket/12121#comment:8
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.3</em> to <em>sage-6.4</em>
</li>
</ul>
TicketzimmermaTue, 07 Jul 2015 12:22:34 GMT
https://trac.sagemath.org/ticket/12121#comment:9
https://trac.sagemath.org/ticket/12121#comment:9
<p>
I'm not sure if this should go to this ticket, but the following never returns:
</p>
<pre class="wiki">sage: z
(11/9*sqrt(3)*sqrt(2) + 3)^(1/3) + 1/3/(11/9*sqrt(3)*sqrt(2) + 3)^(1/3) - 1
sage: floor(z)
</pre><p>
Even <code>floor(z, maximum_bits=53)</code> loops infinitely.
Should I open a separate ticket?
</p>
Ticketajagekar.akshayThu, 11 Feb 2016 20:15:53 GMTbranch set
https://trac.sagemath.org/ticket/12121#comment:10
https://trac.sagemath.org/ticket/12121#comment:10
<ul>
<li><strong>branch</strong>
set to <em>u/ajagekar.akshay/Trac12121</em>
</li>
</ul>
Ticketajagekar.akshayThu, 11 Feb 2016 20:17:44 GMTstatus changed; commit, author set
https://trac.sagemath.org/ticket/12121#comment:11
https://trac.sagemath.org/ticket/12121#comment:11
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>6b75d32ab5cc279a8566f0bc67acc2501bbb833e</em>
</li>
<li><strong>author</strong>
set to <em>Akshay Ajagekar</em>
</li>
</ul>
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=6b75d32ab5cc279a8566f0bc67acc2501bbb833e"><span class="icon"></span>6b75d32</a></td><td><code>Trac #12121: floor/ceil can be very slow at integral values</code>
</td></tr></table>
Ticketajagekar.akshayMon, 29 Feb 2016 15:18:12 GMTcommit, branch, author deleted
https://trac.sagemath.org/ticket/12121#comment:12
https://trac.sagemath.org/ticket/12121#comment:12
<ul>
<li><strong>commit</strong>
<em>6b75d32ab5cc279a8566f0bc67acc2501bbb833e</em> deleted
</li>
<li><strong>branch</strong>
<em>u/ajagekar.akshay/Trac12121</em> deleted
</li>
<li><strong>author</strong>
<em>Akshay Ajagekar</em> deleted
</li>
</ul>
TicketvdelecroixWed, 09 Mar 2016 18:53:08 GMT
https://trac.sagemath.org/ticket/12121#comment:13
https://trac.sagemath.org/ticket/12121#comment:13
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:9" title="Comment 9">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
I'm not sure if this should go to this ticket, but the following never returns:
</p>
<pre class="wiki">sage: z
(11/9*sqrt(3)*sqrt(2) + 3)^(1/3) + 1/3/(11/9*sqrt(3)*sqrt(2) + 3)^(1/3) - 1
sage: floor(z)
</pre><p>
Even <code>floor(z, maximum_bits=53)</code> loops infinitely.
</p>
</blockquote>
<p>
Whereas the following actually works
</p>
<pre class="wiki">sage: bool(z == 1)
True
</pre><blockquote class="citation">
<p>
Should I open a separate ticket?
</p>
</blockquote>
<p>
I think that "very slow" includes "infinite amount of time". For me it is worth it to also fix this kind of endless loops in this ticket.
</p>
TicketvdelecroixWed, 09 Mar 2016 18:56:16 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/12121#comment:14
https://trac.sagemath.org/ticket/12121#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-6.4</em> to <em>sage-7.1</em>
</li>
</ul>
<p>
@ajagekar.akshay: why did you remove your branch? The commit lacks some examples (as the one of <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:9" title="Comment 9">comment:9</a>).
</p>
Ticketajagekar.akshayWed, 09 Mar 2016 19:06:20 GMT
https://trac.sagemath.org/ticket/12121#comment:15
https://trac.sagemath.org/ticket/12121#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:14" title="Comment 14">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
@ajagekar.akshay: why did you remove your branch? The commit lacks some examples (as the one of <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:9" title="Comment 9">comment:9</a>).
</p>
</blockquote>
<p>
Sorry but I pushed that branch without testing, some tests failed.
</p>
TicketvdelecroixWed, 09 Mar 2016 19:08:53 GMT
https://trac.sagemath.org/ticket/12121#comment:16
https://trac.sagemath.org/ticket/12121#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:15" title="Comment 15">ajagekar.akshay</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:14" title="Comment 14">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
@ajagekar.akshay: why did you remove your branch? The commit lacks some examples (as the one of <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:9" title="Comment 9">comment:9</a>).
</p>
</blockquote>
<p>
Sorry but I pushed that branch without testing, some tests failed.
</p>
</blockquote>
<p>
That is not a problem. Tickets can have different status: <code>closed</code>, <code>positive_review</code>, <code>needs_review</code>, <code>needs_work</code>, <code>new</code>. If there is no branch associated to a ticket it makes no sense to keep it into "needs review" state. (I already changed it to needs_work)
</p>
TicketvdelecroixWed, 09 Mar 2016 19:14:08 GMT
https://trac.sagemath.org/ticket/12121#comment:17
https://trac.sagemath.org/ticket/12121#comment:17
<p>
Indeed, your code was good... and there is a bug somewhere else in the conversion from <code>SR</code> to the real interval fields:
</p>
<pre class="wiki">sage: RealIntervalField(256)(SR(10^50 + 10^(-50))).is_int()
(True, 100000000000000000000000000000000000000000000000000)
</pre><p>
Sorry, I was wrong. The method <code>is_int</code> is <strong>not</strong> intended to test if the interval is actually restricted to one integer! It only tests if the interval contains only one integer.
</p>
TicketvdelecroixWed, 09 Mar 2016 19:46:23 GMTstatus changed; commit, branch, author set
https://trac.sagemath.org/ticket/12121#comment:18
https://trac.sagemath.org/ticket/12121#comment:18
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>9821cadaa866f28a0178ce4c75b03dbc82aabd30</em>
</li>
<li><strong>branch</strong>
set to <em>u/vdelecroix/12121</em>
</li>
<li><strong>author</strong>
set to <em>Vincent Delecroix</em>
</li>
</ul>
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=9821cadaa866f28a0178ce4c75b03dbc82aabd30"><span class="icon"></span>9821cad</a></td><td><code>Trac 12121: fix floor/ceil functions</code>
</td></tr></table>
TicketgitWed, 09 Mar 2016 19:58:27 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:19
https://trac.sagemath.org/ticket/12121#comment:19
<ul>
<li><strong>commit</strong>
changed from <em>9821cadaa866f28a0178ce4c75b03dbc82aabd30</em> to <em>800d0ee77a1628f92faaf1b1e159d4aa3f1ed966</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="http://git.sagemath.org/sage.git/commit/?id=800d0ee77a1628f92faaf1b1e159d4aa3f1ed966"><span class="icon"></span>800d0ee</a></td><td><code>Trac 12121: fix floor/ceil functions</code>
</td></tr></table>
TicketgitThu, 10 Mar 2016 02:44:19 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:20
https://trac.sagemath.org/ticket/12121#comment:20
<ul>
<li><strong>commit</strong>
changed from <em>800d0ee77a1628f92faaf1b1e159d4aa3f1ed966</em> to <em>50bdaf3b31aecc326045e48524d2aa4d98186549</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="http://git.sagemath.org/sage.git/commit/?id=50bdaf3b31aecc326045e48524d2aa4d98186549"><span class="icon"></span>50bdaf3</a></td><td><code>Trac 12121: work around a bug with symbolics</code>
</td></tr></table>
TicketvdelecroixThu, 10 Mar 2016 02:45:38 GMT
https://trac.sagemath.org/ticket/12121#comment:21
https://trac.sagemath.org/ticket/12121#comment:21
<p>
I pushed a tiny fix because we currently have
</p>
<pre class="wiki">sage: log(8) / log(2) == 3
False
</pre><p>
See the discussion at <a class="ext-link" href="https://groups.google.com/forum/#!topic/sage-devel/xAlQmUF_cdU"><span class="icon"></span>this sage-devel thread</a>.
</p>
TicketmmezzarobbaThu, 24 Mar 2016 14:35:11 GMT
https://trac.sagemath.org/ticket/12121#comment:22
https://trac.sagemath.org/ticket/12121#comment:22
<p>
<a class="closed ticket" href="https://trac.sagemath.org/ticket/15786" title="defect: floor fails for certain expressions. (closed: worksforme)">#15786</a> is a partial duplicate. I find the fix posted there a bit cleaner, though it needs to be rebased.
</p>
TicketvdelecroixThu, 24 Mar 2016 14:41:43 GMT
https://trac.sagemath.org/ticket/12121#comment:23
https://trac.sagemath.org/ticket/12121#comment:23
<p>
Rebased on what!?
</p>
<p>
There is one thing I am not happy with. I think we should try to make the interval much smaller than 1 in
</p>
<pre class="wiki">while x_interval.absolute_diameter() >= 1:
bits *= 2
x_interval = RealIntervalField(bits)(x)
</pre><p>
before trying to simplify the expression. I guess that 2<sup>-30</sup> would be much more reasonable and avoid many attempts of (costly) simplification. What do you think?
</p>
TicketmmezzarobbaThu, 24 Mar 2016 14:46:23 GMT
https://trac.sagemath.org/ticket/12121#comment:24
https://trac.sagemath.org/ticket/12121#comment:24
<p>
Sorry if I wasn't clear. I'm saying that the <em>other</em> branch (the one at <a class="closed ticket" href="https://trac.sagemath.org/ticket/15786" title="defect: floor fails for certain expressions. (closed: worksforme)">#15786</a>) should be rebased.
</p>
TicketrwsThu, 24 Mar 2016 14:59:09 GMT
https://trac.sagemath.org/ticket/12121#comment:25
https://trac.sagemath.org/ticket/12121#comment:25
<p>
Be aware you might be using symbolics when doing bool( == ). To not end up with surprises you might want to break out the actual functionality you need out of <code>Expression.__nonzero__</code> or review <a class="closed ticket" href="https://trac.sagemath.org/ticket/16397" title="defect: Symbolic cmp (closed: fixed)">#16397</a> and use <code>cmp</code>.
</p>
TicketgitThu, 24 Mar 2016 15:52:50 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:26
https://trac.sagemath.org/ticket/12121#comment:26
<ul>
<li><strong>commit</strong>
changed from <em>50bdaf3b31aecc326045e48524d2aa4d98186549</em> to <em>688ebfa077f09ee9a2bc988833ddd4f0fc65a4a1</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="http://git.sagemath.org/sage.git/commit/?id=688ebfa077f09ee9a2bc988833ddd4f0fc65a4a1"><span class="icon"></span>688ebfa</a></td><td><code>Trac 12121: code improvements</code>
</td></tr></table>
TicketvdelecroixThu, 24 Mar 2016 15:55:02 GMT
https://trac.sagemath.org/ticket/12121#comment:27
https://trac.sagemath.org/ticket/12121#comment:27
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:25" title="Comment 25">rws</a>:
</p>
<blockquote class="citation">
<p>
Be aware you might be using symbolics when doing bool( == ). To not end up with surprises you might want to break out the actual functionality you need out of <code>Expression.__nonzero__</code> or review <a class="closed ticket" href="https://trac.sagemath.org/ticket/16397" title="defect: Symbolic cmp (closed: fixed)">#16397</a> and use <code>cmp</code>.
</p>
</blockquote>
<p>
You mean that there is no way to check that <code>expr1 == expr2</code>? I do not want to copy/paste anything from other place. If testing equality is not available, there is a big problem.
</p>
<p>
Using <code>cmp</code> for that purpose is a bad idea. The purpose of <code>cmp</code> in Python 2 is to sort things out. Not to compare.
</p>
TicketrwsThu, 24 Mar 2016 16:11:44 GMT
https://trac.sagemath.org/ticket/12121#comment:28
https://trac.sagemath.org/ticket/12121#comment:28
<p>
Testing equality of symbolics in general is undecidable. If you remove all expressions with variables however, it is easy: convert symbolic constants and function expressions to float as in <code>N()</code>, compare. Of course there are precision problems but that's nothing in comparison to what you get with variables.
</p>
<p>
The idea to abuse <code>cmp</code> is not mine. Somewhere here <code>RLF(1) < RLF(sqrt(2))</code> for example, symbolic <code>cmp</code> is called. Should I file a bug report for such usage?
</p>
<p>
EDIT: typos
</p>
TicketmmezzarobbaThu, 24 Mar 2016 16:19:05 GMT
https://trac.sagemath.org/ticket/12121#comment:29
https://trac.sagemath.org/ticket/12121#comment:29
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:28" title="Comment 28">rws</a>:
</p>
<blockquote class="citation">
<p>
Testing equality of symbolics in general is undecidable. If you remove all expressions with variables however, it is easy
</p>
</blockquote>
<p>
It is unknown if it is decidable (afaik) even without variables.
</p>
<blockquote class="citation">
<p>
The idea to abuse <code>cmp</code> is not mine. Somewhere here <code>RLF(1) < RLF(sqrt(2))</code> for example, symbolic <code>cmp</code> is called. Should I file a bug report for such usage?
</p>
</blockquote>
<p>
I'd say it probably is a bug. As to whether you should file a bug report, I don't know—I doubt anyone really reads them, and the issue is probably already covered somewhere in the myriad of known bugs with comparisons...
</p>
TicketvdelecroixThu, 24 Mar 2016 16:20:13 GMT
https://trac.sagemath.org/ticket/12121#comment:30
https://trac.sagemath.org/ticket/12121#comment:30
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:28" title="Comment 28">rws</a>:
</p>
<blockquote class="citation">
<p>
Testing equality of symbolics in general is undecidable. If you remove all expressions with variables however, it is easy: convert symbolic constants and function expressions to float as in <code>N()</code>, compare. Of course there are precision problems but that's nothing in comparison to what you get with variables.
</p>
</blockquote>
<p>
What I need is a method that either returns a reliable answer <code>True</code> or <code>False</code> or raise an error which can either be <code>This comparison is meaningless</code> or <code>I don't know how to compare this</code>.
</p>
TicketrwsThu, 24 Mar 2016 16:39:57 GMT
https://trac.sagemath.org/ticket/12121#comment:31
https://trac.sagemath.org/ticket/12121#comment:31
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:30" title="Comment 30">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
What I need is a method that either returns a reliable answer <code>True</code> or <code>False</code> or raise an error which can either be <code>This comparison is meaningless</code> or <code>I don't know how to compare this</code>.
</p>
</blockquote>
<p>
Then <code>bool( == )</code> is right for the moment and may be replaced with <a class="needs_info ticket" href="https://trac.sagemath.org/ticket/19040" title="enhancement: rewrite Expression.__nonzero__() (needs_info)">#19040</a>. As said don't be surprised if it takes a long time.
</p>
TicketgitThu, 24 Mar 2016 18:20:55 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:32
https://trac.sagemath.org/ticket/12121#comment:32
<ul>
<li><strong>commit</strong>
changed from <em>688ebfa077f09ee9a2bc988833ddd4f0fc65a4a1</em> to <em>a5ef94a5b27b302d813ac83538558b39a39c7267</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="http://git.sagemath.org/sage.git/commit/?id=a5ef94a5b27b302d813ac83538558b39a39c7267"><span class="icon"></span>a5ef94a</a></td><td><code>Trac 12121: note about #19040 + extra if</code>
</td></tr></table>
TicketvdelecroixThu, 24 Mar 2016 18:21:53 GMT
https://trac.sagemath.org/ticket/12121#comment:33
https://trac.sagemath.org/ticket/12121#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:31" title="Comment 31">rws</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:30" title="Comment 30">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
What I need is a method that either returns a reliable answer <code>True</code> or <code>False</code> or raise an error which can either be <code>This comparison is meaningless</code> or <code>I don't know how to compare this</code>.
</p>
</blockquote>
<p>
Then <code>bool( == )</code> is right for the moment and may be replaced with <a class="needs_info ticket" href="https://trac.sagemath.org/ticket/19040" title="enhancement: rewrite Expression.__nonzero__() (needs_info)">#19040</a>. As said don't be surprised if it takes a long time.
</p>
</blockquote>
<p>
I added a note about <a class="needs_info ticket" href="https://trac.sagemath.org/ticket/19040" title="enhancement: rewrite Expression.__nonzero__() (needs_info)">#19040</a>.
</p>
TicketvdelecroixThu, 24 Mar 2016 18:36:02 GMTdescription, milestone changed
https://trac.sagemath.org/ticket/12121#comment:34
https://trac.sagemath.org/ticket/12121#comment:34
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/12121?action=diff&version=34">diff</a>)
</li>
<li><strong>milestone</strong>
changed from <em>sage-7.1</em> to <em>sage-7.2</em>
</li>
</ul>
TicketmmezzarobbaFri, 25 Mar 2016 15:59:45 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/12121#comment:35
https://trac.sagemath.org/ticket/12121#comment:35
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>Marc Mezzarobba</em>
</li>
</ul>
<p>
Hi Vincent,
</p>
<p>
Sorry for my unclear comments above, I didn't look closely enough at your code before posting them.
</p>
<p>
Beside the issue with comparisons raised by Ralf, I find the code on your branch a bit complicated, and I don't like the fact that you drop the <code>maximum_bits</code> parameters at the risk of looping forever if you cannot prove that the input is an integer. I pushed to <code>u/mmezzarobba/12121-ceil</code> a rough attempt to fix these issues (in the case of <code>ceil</code> only at the moment, and not well tested yet), please tell me what you think of it.
</p>
TicketgitTue, 05 Apr 2016 21:11:26 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:36
https://trac.sagemath.org/ticket/12121#comment:36
<ul>
<li><strong>commit</strong>
changed from <em>a5ef94a5b27b302d813ac83538558b39a39c7267</em> to <em>60f0c291adf28344156661ea7623c5721ed4a06b</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="http://git.sagemath.org/sage.git/commit/?id=60f0c291adf28344156661ea7623c5721ed4a06b"><span class="icon"></span>60f0c29</a></td><td><code>Trac 12121: Fix floor/ceil</code>
</td></tr></table>
TicketvdelecroixTue, 05 Apr 2016 21:12:38 GMTstatus changed
https://trac.sagemath.org/ticket/12121#comment:37
https://trac.sagemath.org/ticket/12121#comment:37
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
All right. I factorized the two implementations in a new function <code>incremental_rounding</code>. It is cleaner and the parameter <code>maximum_bits</code> is reintroduced.
</p>
TicketgitWed, 06 Apr 2016 01:57:23 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:38
https://trac.sagemath.org/ticket/12121#comment:38
<ul>
<li><strong>commit</strong>
changed from <em>60f0c291adf28344156661ea7623c5721ed4a06b</em> to <em>d53c406171d73a0138d25336732728b41409c8b9</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="http://git.sagemath.org/sage.git/commit/?id=d53c406171d73a0138d25336732728b41409c8b9"><span class="icon"></span>d53c406</a></td><td><code>Trac 12121: Fix floor/ceil</code>
</td></tr></table>
TicketmmezzarobbaWed, 06 Apr 2016 08:41:07 GMT
https://trac.sagemath.org/ticket/12121#comment:39
https://trac.sagemath.org/ticket/12121#comment:39
<p>
I fear I won't have time to review your new version in the next few days at least, but from a quick look at it there are a lot of things I don't understand. In no particular order:
</p>
<ul><li>why do you make <code>maximum_bits</code> an <code>Integer</code>?
</li><li>what don't you like about <code>unique_integer()</code>?
</li><li>is it really better to have an absolute bound for the diameter that makes us suspect we found an exact integer, rather than something that depends on the precision?
</li><li>why do you insist on using <code>==</code> on symbolic expressions instead of <code>is_trivial_zero()</code>?
</li><li>are you sure you want to raise an error when <code>maximum_bits</code> does not suffice to conclude? this is a symbolic function that may be buried deep in the middle of a symbolic expression; returning unevaluated seems more reasonable to me...
</li><li>do we really need two loops that do essentially the same thing (including raising errors with the exact same message)?
</li></ul>
TicketgitWed, 06 Apr 2016 17:17:23 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:40
https://trac.sagemath.org/ticket/12121#comment:40
<ul>
<li><strong>commit</strong>
changed from <em>d53c406171d73a0138d25336732728b41409c8b9</em> to <em>0e9b2f21d6a6bc42e35302316ac0f6b3b7451450</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="http://git.sagemath.org/sage.git/commit/?id=0e9b2f21d6a6bc42e35302316ac0f6b3b7451450"><span class="icon"></span>0e9b2f2</a></td><td><code>Trac 12121: remove __call__ and fix round</code>
</td></tr></table>
TicketvdelecroixWed, 06 Apr 2016 17:18:35 GMT
https://trac.sagemath.org/ticket/12121#comment:41
https://trac.sagemath.org/ticket/12121#comment:41
<p>
I removed the <code>__call__</code> in both <code>Function_floor</code> and <code>Function_ceil</code>. The code is now much simpler. Though there was some adaptation needed in <code>symbolic/expression.pyx</code>.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:39" title="Comment 39">mmezzarobba</a>:
</p>
<blockquote class="citation">
<p>
I fear I won't have time to review your new version in the next few days at least, but from a quick look at it there are a lot of things I don't understand. In no particular order:
</p>
<ul><li>why do you make <code>maximum_bits</code> an <code>Integer</code>?
</li></ul></blockquote>
<p>
all right. <code>int</code> is fine as well.
</p>
<blockquote class="citation">
<ul><li>what don't you like about <code>unique_integer()</code>?
</li></ul></blockquote>
<p>
an <code>assert</code> does not cost anything. And <code>unique_integer</code> silently fails if the interval does not enclose a unique integer.
</p>
<blockquote class="citation">
<ul><li>is it really better to have an absolute bound for the diameter that makes us suspect we found an exact integer, rather than something that depends on the precision?
</li></ul></blockquote>
<p>
the precision of what? there is the field used for the evaluation which is different from the diameter of the interval. If you have more than one integer in your interval which one are you using to test equality?
</p>
<blockquote class="citation">
<ul><li>why do you insist on using <code>==</code> on symbolic expressions instead of <code>is_trivial_zero()</code>?
</li></ul></blockquote>
<p>
Because I want to check equality with an integer. Not if it is a trivial equality.
</p>
<blockquote class="citation">
<ul><li>are you sure you want to raise an error when <code>maximum_bits</code> does not suffice to conclude? this is a symbolic function that may be buried deep in the middle of a symbolic expression; returning unevaluated seems more reasonable to me...
</li></ul></blockquote>
<p>
Done with an example.
</p>
<blockquote class="citation">
<ul><li>do we really need two loops that do essentially the same thing (including raising errors with the exact same message)?
</li></ul></blockquote>
<p>
The equality test is potentially costly. And we want to avoid it as much as possible. In particular, it makes no sense to test this equality within each step of the loop as it is in your version. On a related note, I noticed that for <code>round</code> you need to test equality with elements of <code>ZZ + 1/2</code>.
</p>
TicketvdelecroixWed, 06 Apr 2016 19:06:24 GMT
https://trac.sagemath.org/ticket/12121#comment:42
https://trac.sagemath.org/ticket/12121#comment:42
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:41" title="Comment 41">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:39" title="Comment 39">mmezzarobba</a>:
</p>
<blockquote class="citation">
<ul><li>do we really need two loops that do essentially the same thing (including raising errors with the exact same message)?
</li></ul></blockquote>
<p>
The equality test is potentially costly. And we want to avoid it as much as possible. In particular, it makes no sense to test this equality within each step of the loop as it is in your version. On a related note, I noticed that for <code>round</code> you need to test equality with elements of <code>ZZ + 1/2</code>.
</p>
</blockquote>
<p>
And I also would like to use the very same function <code>incremental_rounding</code> for elements of <code>QQbar</code>. For the very same reason, you only want very lately the equality test.
</p>
TicketgitWed, 06 Apr 2016 19:31:47 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:43
https://trac.sagemath.org/ticket/12121#comment:43
<ul>
<li><strong>commit</strong>
changed from <em>0e9b2f21d6a6bc42e35302316ac0f6b3b7451450</em> to <em>313c497daaec6324dfe4ffdeb684844e301a3f61</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="http://git.sagemath.org/sage.git/commit/?id=313c497daaec6324dfe4ffdeb684844e301a3f61"><span class="icon"></span>313c497</a></td><td><code>Trac 12121: fix doctests</code>
</td></tr></table>
TicketvdelecroixWed, 06 Apr 2016 19:36:52 GMTdescription changed
https://trac.sagemath.org/ticket/12121#comment:44
https://trac.sagemath.org/ticket/12121#comment:44
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/12121?action=diff&version=44">diff</a>)
</li>
</ul>
TicketvdelecroixThu, 28 Apr 2016 22:49:33 GMT
https://trac.sagemath.org/ticket/12121#comment:45
https://trac.sagemath.org/ticket/12121#comment:45
<p>
ping?!
</p>
TicketmmezzarobbaFri, 29 Apr 2016 11:27:34 GMT
https://trac.sagemath.org/ticket/12121#comment:46
https://trac.sagemath.org/ticket/12121#comment:46
<p>
Sorry, I have about zero time for Sage development before at least 1-2 weeks. All I can say is that I wasn't completely convinced by your answers and would need to think things over more carefully. If someone wants to review the ticket in the meantime, please do.
</p>
TicketgitSun, 08 May 2016 02:45:35 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:47
https://trac.sagemath.org/ticket/12121#comment:47
<ul>
<li><strong>commit</strong>
changed from <em>313c497daaec6324dfe4ffdeb684844e301a3f61</em> to <em>602e5158c5f37c6900a1dee8e27a228114af1319</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="http://git.sagemath.org/sage.git/commit/?id=9bfef80cf0a665960772a74505f9c9158db84163"><span class="icon"></span>9bfef80</a></td><td><code>Trac 12121: Fix floor/ceil</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=c8abe9c9f1c3d9c47f0468ee355cf6bd14643ebe"><span class="icon"></span>c8abe9c</a></td><td><code>Trac 12121: remove __call__ and fix round</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=602e5158c5f37c6900a1dee8e27a228114af1319"><span class="icon"></span>602e515</a></td><td><code>Trac 12121: fix doctests</code>
</td></tr></table>
TicketvdelecroixSun, 08 May 2016 02:45:56 GMT
https://trac.sagemath.org/ticket/12121#comment:48
https://trac.sagemath.org/ticket/12121#comment:48
<p>
rebased on 7-2.rc1
</p>
TicketmmezzarobbaTue, 17 May 2016 13:42:19 GMTstatus changed
https://trac.sagemath.org/ticket/12121#comment:49
https://trac.sagemath.org/ticket/12121#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Okay, I'm back. Sorry that it took so long.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:41" title="Comment 41">vdelecroix</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>what don't you like about <code>unique_integer()</code>?
</li></ul></blockquote>
<p>
an <code>assert</code> does not cost anything. And <code>unique_integer</code> silently fails if the interval does not enclose a unique integer.
</p>
</blockquote>
<p>
Uh? No, it doesn't.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>is it really better to have an absolute bound for the diameter that makes us suspect we found an exact integer, rather than something that depends on the precision?
</li></ul></blockquote>
<p>
the precision of what? there is the field used for the evaluation which is different from the diameter of the interval. If you have more than one integer in your interval which one are you using to test equality?
</p>
</blockquote>
<p>
The precision of the interval computation, i.e. <code>bits</code>. The idea being that if we found an interval of width (say) 2⁻²⁰ containing an integer by computing with 1000 bits of precision, we may want to see if the width of the interval keeps decreasing when the precision increases and only run the symbolic part of the algorithm if that's the case. But I agree that this is a very minor issue at best.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>why do you insist on using <code>==</code> on symbolic expressions instead of <code>is_trivial_zero()</code>?
</li></ul></blockquote>
<p>
Because I want to check equality with an integer. Not if it is a trivial equality.
</p>
</blockquote>
<p>
I mean using <code>is_trivial_zero()</code> after calling <code>full_simplify()</code> & co, as in my version: this is safer than relying on <code>==</code> and essentially as powerful.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>do we really need two loops that do essentially the same thing (including raising errors with the exact same message)?
</li></ul></blockquote>
<p>
The equality test is potentially costly. And we want to avoid it as much as possible. In particular, it makes no sense to test this equality within each step of the loop as it is in your version.
</p>
</blockquote>
<p>
Yes—as I said, my version was just a rough sketch of the changes I'd b tempted to make, nothing finished. As the code I was starting with used to loop forever in the typical situation where mine would do the symbolic test repeatedly (that is, <code>x</code> ∈ ℤ integer but we don't manage to prove it), I thought the additional cost would be acceptable. <code>;-)</code> But anyway, it is not hard to do the test only once while avoiding the code duplication.
</p>
<blockquote class="citation">
<p>
On a related note, I noticed that for <code>round</code> you need to test equality with elements of <code>ZZ + 1/2</code>.
</p>
</blockquote>
<p>
Couldn't you just compute ceil(x-1/2)?
</p>
<hr />
<p>
Now for some comments on the current code:
</p>
<ul><li>To summarize the above, I still think the main logic in <code>incremental_rounding()</code> could be shortened to something like (not tested):
<pre class="wiki"> unique_rounding = getattr(RealIntervalFieldElement, 'unique_' + mode)
r = RR.one() >> 20
bits = 64
candidate = None
while bits < maximum_bits:
interval = RealIntervalField(bits)(x) # may raise a TypeError
try:
return unique_rounding(interval)
except ValueError:
pass
if candidate is None and interval.absolute_diameter() > r:
candidate = interval.unique_integer()
try:
delta = x - candidate
if (delta.is_zero()
or SR(delta).full_simplify().canonicalize_radical()
.is_trivial_zero()
or QQbar(delta).is_zero()):
return candidate
except (TypeError, ValueError):
pass
bits *= 2
</pre></li></ul><ul><li>I'd also make <code>incremental_rounding()</code> private and dispense with the argument checking (and perhaps move it to <code>real_mpfi</code> if your plan is to use it from elsewhere)—but I'm okay with keeping it as it is. An advantage of making it private is that you could take the “unique rounding” function on intervals as input instead of accessing it via <code>getattr()</code>. Another option would be to introduce a common base class for <code>Function_floor</code>, <code>Function_ceil</code> and <code>Function_round</code>.
</li></ul><ul><li>I'm a little uneasy about the changes you made to <code>BuiltinFunction.__call__()</code>. The fact that it used to convert non-Element inputs to Elements looks intentional and pretty reasonable to me. Is it really necessary to change that behavior? That being said, I'm a bit lost in the maze of <code>Function.__call__</code>, <code>BuiltinFunction.__call__</code>, <code>_eval_</code>, <code>_evalf_</code>, <code>_evalf_try_</code> and friends, so if you tell me you are confident that the change is correct I'll trust you!
</li></ul><ul><li>If these changes stay, then I guess this
<pre class="wiki">if module is not None:
func = getattr(module, self._name, None)
if func is None and self._alt_name is not None:
func = getattr(module, self._name, None)
^^^^^
</pre>should be <code>_alt_name</code>.
</li></ul><ul><li>Note that these changes also make
<pre class="wiki">sage: sin(numpy.int32(0))
0.0
</pre>which is at odds with
<pre class="wiki">sage: sin(ZZ(0)).parent()
Integer Ring
</pre>(perhaps not ideal, but predictable at least).
</li></ul><ul><li>In <code>Function_*</code>, what is the point of calling <code>_evalf_()</code> from <code>_eval_()</code>?
</li></ul><ul><li>And why isn't the logic for choosing <code>maximum_bits</code> in <code>_evalf_()</code> the same in <code>floor</code>, <code>ceil</code> and <code>round</code>?
</li></ul><ul><li>I wouldn't bother with checking that <code>x</code> is not a relation. First, <code>_eval_()</code> methods of individual functions are probably not the right place for that (either <code>BuiltinFunction.__call__()</code> or perhaps methods <code>ceil()</code>, <code>floor()</code> etc. in a future subclass <code>RelationalExpression</code> of <code>Expression</code> would be more reasonable). Besides, various other nonsensical inputs (e.g. series, booleans) are accepted without error, so it is a bit strange to have an ad hoc check dealing with this one.
</li></ul>
TicketnbruinThu, 19 May 2016 02:38:44 GMT
https://trac.sagemath.org/ticket/12121#comment:50
https://trac.sagemath.org/ticket/12121#comment:50
<p>
You may be interested in <a class="closed ticket" href="https://trac.sagemath.org/ticket/20624" title="defect: maximum recursion depth exceeded in MonoDictEraser (closed: fixed)">#20624</a>. It looks like implementing <code>_evalf_</code> by calling <code>_eval_</code> is VERY bad: currently evaluation of <code>ceil</code> may lead to running out the python call stack before doing anything useful. Obviously, this takes some time. Inheriting from <code>BuiltinFunction</code> is a real bugtrap: its init reassigns a whole bunch of methods.
</p>
TicketrwsThu, 19 May 2016 05:39:25 GMTcc changed
https://trac.sagemath.org/ticket/12121#comment:51
https://trac.sagemath.org/ticket/12121#comment:51
<ul>
<li><strong>cc</strong>
<em>kcrisman.</em> <em>jdemeyer</em> added; <em>kcrisman</em> removed
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:50" title="Comment 50">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Inheriting from <code>BuiltinFunction</code> is a real bugtrap: its init reassigns a whole bunch of methods.
</p>
</blockquote>
<p>
That must be the reason why Sage crashes all the time. Seriously, I agree the construction of functions is messy and it limits the developer somewhat, see "other symbolic function tickets" in <a class="ext-link" href="http://trac.sagemath.org/wiki/symbolics/functions"><span class="icon"></span>http://trac.sagemath.org/wiki/symbolics/functions</a>. Note also there are a bunch of tickets needing review there. However, I think the design is sound. You just have to read how other functions (the more recently implemented) are using it. In the end, I or someone will be transferring the Python you write to Pynac, anyway.
</p>
<p>
Cc: the author of the <code>_evalf_try_</code> mechanism.
</p>
TicketrwsSat, 11 Jun 2016 15:37:01 GMTbranch changed
https://trac.sagemath.org/ticket/12121#comment:52
https://trac.sagemath.org/ticket/12121#comment:52
<ul>
<li><strong>branch</strong>
changed from <em>u/vdelecroix/12121</em> to <em>public/12121</em>
</li>
</ul>
TicketrwsSat, 11 Jun 2016 15:43:43 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:53
https://trac.sagemath.org/ticket/12121#comment:53
<ul>
<li><strong>commit</strong>
changed from <em>602e5158c5f37c6900a1dee8e27a228114af1319</em> to <em>9bd70406da71c6e1083fc25cd57435788689018e</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:50" title="Comment 50">nbruin</a>:
</p>
<blockquote class="citation">
<p>
You may be interested in <a class="closed ticket" href="https://trac.sagemath.org/ticket/20624" title="defect: maximum recursion depth exceeded in MonoDictEraser (closed: fixed)">#20624</a>. It looks like implementing <code>_evalf_</code> by calling <code>_eval_</code> is VERY bad: currently evaluation of <code>ceil</code> may lead to running out the python call stack before doing anything useful.
</p>
</blockquote>
<p>
I may be mistaken but actually <code>_eval_</code> calls <code>_evalf_</code> here which is a completely different matter.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=9bd70406da71c6e1083fc25cd57435788689018e"><span class="icon"></span>9bd7040</a></td><td><code>Merge branch 'develop' into t/12121/12121</code>
</td></tr></table>
TicketvdelecroixWed, 13 Jul 2016 15:48:41 GMT
https://trac.sagemath.org/ticket/12121#comment:54
https://trac.sagemath.org/ticket/12121#comment:54
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:49" title="Comment 49">mmezzarobba</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
On a related note, I noticed that for <code>round</code> you need to test equality with elements of <code>ZZ + 1/2</code>.
</p>
</blockquote>
<p>
Couldn't you just compute ceil(x-1/2)?
</p>
</blockquote>
<pre class="wiki">sage: x = 0.5
sage: print x.round() == (x-0.5).ceil()
False
</pre>
TicketvdelecroixWed, 13 Jul 2016 16:23:00 GMT
https://trac.sagemath.org/ticket/12121#comment:55
https://trac.sagemath.org/ticket/12121#comment:55
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:49" title="Comment 49">mmezzarobba</a>:
</p>
<blockquote class="citation">
<ul><li>In <code>Function_*</code>, what is the point of calling <code>_evalf_()</code> from <code>_eval_()</code>?
</li></ul></blockquote>
<p>
Because I want the answer of <code>floor(pi)</code> to be <code>3</code>.
</p>
TicketvdelecroixWed, 13 Jul 2016 16:39:19 GMTcc, status, milestone changed
https://trac.sagemath.org/ticket/12121#comment:56
https://trac.sagemath.org/ticket/12121#comment:56
<ul>
<li><strong>cc</strong>
<em>kcrisman</em> added; <em>kcrisman.</em> removed
</li>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-7.2</em> to <em>sage-7.3</em>
</li>
</ul>
TicketvdelecroixWed, 13 Jul 2016 16:39:50 GMTcommit, description, branch changed
https://trac.sagemath.org/ticket/12121#comment:57
https://trac.sagemath.org/ticket/12121#comment:57
<ul>
<li><strong>commit</strong>
changed from <em>9bd70406da71c6e1083fc25cd57435788689018e</em> to <em>6f392b8e7b2d562debabca7f99fa1b54180f83cb</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/12121?action=diff&version=57">diff</a>)
</li>
<li><strong>branch</strong>
changed from <em>public/12121</em> to <em>u/vdelecroix/12121</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=08ecc069c3f88c11060df2ca2383bcb92b2caf27"><span class="icon"></span>08ecc06</a></td><td><code>Trac 12121: Fix floor/ceil</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=b2c5fbe9501e43e6e5e117aae4412db1f21bf1f2"><span class="icon"></span>b2c5fbe</a></td><td><code>Trac 12121: change __call__ and workaround</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=82ad3039d0913329de3e3ed514c6397483b7b93a"><span class="icon"></span>82ad303</a></td><td><code>Trac 12121: fix doctests</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=194ba9976c58c367d01b767f405f4b4dae1dcad7"><span class="icon"></span>194ba99</a></td><td><code>Trac 12121: _evalf_ more consistent</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=2719621b352b267d4aa31d95d8f0c22c08cd1c73"><span class="icon"></span>2719621</a></td><td><code>Trac 12121: do not check for relation</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=6f392b8e7b2d562debabca7f99fa1b54180f83cb"><span class="icon"></span>6f392b8</a></td><td><code>Trac 12121: fix ._name -> ._alt_name</code>
</td></tr></table>
TicketrwsThu, 14 Jul 2016 06:08:02 GMTreviewer changed
https://trac.sagemath.org/ticket/12121#comment:58
https://trac.sagemath.org/ticket/12121#comment:58
<ul>
<li><strong>reviewer</strong>
changed from <em>Marc Mezzarobba</em> to <em>Marc Mezzarobba, Ralf Stephan</em>
</li>
</ul>
<p>
<code>Function</code> mechanics looking very good. Can't comment on the <code>incremental_rounding()</code> function part.
</p>
TicketvdelecroixWed, 10 Aug 2016 23:16:56 GMTstatus changed; dependencies set
https://trac.sagemath.org/ticket/12121#comment:59
https://trac.sagemath.org/ticket/12121#comment:59
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>dependencies</strong>
set to <em>#21216</em>
</li>
</ul>
TicketgitWed, 17 Aug 2016 21:54:02 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:60
https://trac.sagemath.org/ticket/12121#comment:60
<ul>
<li><strong>commit</strong>
changed from <em>6f392b8e7b2d562debabca7f99fa1b54180f83cb</em> to <em>f66febd75b8702831db61585d3591575d024c23b</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=d2f05f1665bd21f5ec708345f4becd48b1c0fc99"><span class="icon"></span>d2f05f1</a></td><td><code>Trac 12121: incremental rounding</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=f66febd75b8702831db61585d3591575d024c23b"><span class="icon"></span>f66febd</a></td><td><code>Trac 12121: use incremental_rounding in floor/ceil/round</code>
</td></tr></table>
TicketvdelecroixWed, 17 Aug 2016 21:55:41 GMTstatus changed; dependencies deleted
https://trac.sagemath.org/ticket/12121#comment:61
https://trac.sagemath.org/ticket/12121#comment:61
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
<em>#21216</em> deleted
</li>
</ul>
<p>
Rebased on the last beta (which includes <a class="closed ticket" href="https://trac.sagemath.org/ticket/21216" title="enhancement: direct function call to math/cmath/mpmath/numpy (closed: fixed)">#21216</a>).
</p>
<p>
I slightly modified <code>incremental_rounding</code>. The simplification part is implemented outside. As soon as there is a reliable <code>is_zero</code> available for elements of SR (as it is the case for <code>QQbar</code> with <code>is_zero</code>) we could make it cleaner.
</p>
TicketmmezzarobbaFri, 16 Sep 2016 09:59:41 GMTstatus changed
https://trac.sagemath.org/ticket/12121#comment:62
https://trac.sagemath.org/ticket/12121#comment:62
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hi Vincent,
</p>
<p>
Sorry for taking so long to reply once again. The code is starting to look really good to me overall, but calling the buggy <code>is_zero()</code> causes regressions such as:
</p>
<pre class="wiki">sage: foo = sin(1 + 10^(-30)) - sin(1)
sage: ceil(foo)
0
sage: floor(foo)
0
</pre><p>
I know we already talked about that above, but are you sure you don't want <code>incremental_rounding()</code> to use <code>is_trivial_zero()</code> (probably after some simplification) instead?
</p>
TicketvdelecroixSat, 17 Sep 2016 07:05:43 GMT
https://trac.sagemath.org/ticket/12121#comment:63
https://trac.sagemath.org/ticket/12121#comment:63
<p>
I definitely want a <code>incremental_rounding</code> that is symbolic ring agnostic. A solution would be to have an optional argument <code>is_zero</code>. What do you think?
</p>
TicketvdelecroixSat, 17 Sep 2016 07:14:26 GMT
https://trac.sagemath.org/ticket/12121#comment:64
https://trac.sagemath.org/ticket/12121#comment:64
<p>
(of course the argument would have default "the_object.is_zero")
</p>
TicketmmezzarobbaSat, 17 Sep 2016 11:09:12 GMT
https://trac.sagemath.org/ticket/12121#comment:65
https://trac.sagemath.org/ticket/12121#comment:65
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:63" title="Comment 63">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
I definitely want a <code>incremental_rounding</code> that is symbolic ring agnostic. A solution would be to have an optional argument <code>is_zero</code>. What do you think?
</p>
</blockquote>
<p>
That sounds good.
</p>
TicketvdelecroixSat, 17 Sep 2016 11:31:46 GMT
https://trac.sagemath.org/ticket/12121#comment:66
https://trac.sagemath.org/ticket/12121#comment:66
<p>
And <code>is_trivial_zero</code> could not be a solution anyway
</p>
<pre class="wiki">sage: delta = (11/9*sqrt(3)*sqrt(2) + 3)^(1/3) + 1/3/(11/9*sqrt(3)*sqrt(2) + 3)^(1/3) - 2
sage: delta.is_zero()
True
sage: delta.is_trivial_zero()
False
sage: delta2 = delta.full_simplify().canonicalize_radical()
sage: delta2.is_zero()
True
sage: delta2.is_trivial_zero()
False
</pre>
TicketvdelecroixSat, 17 Sep 2016 11:44:28 GMT
https://trac.sagemath.org/ticket/12121#comment:67
https://trac.sagemath.org/ticket/12121#comment:67
<p>
Even better
</p>
<pre class="wiki">sage: (sin(1 - 10^(-100)) - sin(1)).is_zero()
True
age: bool(sin(1 - 10^(-100)) - sin(1) == 0)
True
</pre><p>
I thought that we should only worry about false negatives...
</p>
TicketvdelecroixSat, 17 Sep 2016 11:50:27 GMT
https://trac.sagemath.org/ticket/12121#comment:68
https://trac.sagemath.org/ticket/12121#comment:68
<p>
See the following thread
</p>
<blockquote>
<p>
<a class="ext-link" href="https://groups.google.com/forum/#!topic/sage-devel/qitYHanQiEo"><span class="icon"></span>https://groups.google.com/forum/#!topic/sage-devel/qitYHanQiEo</a>
</p>
</blockquote>
TicketgitSat, 17 Sep 2016 13:51:29 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:69
https://trac.sagemath.org/ticket/12121#comment:69
<ul>
<li><strong>commit</strong>
changed from <em>f66febd75b8702831db61585d3591575d024c23b</em> to <em>83c025750342ffbc079e5d8c0c004d2f1b443f22</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=0ef7b9e16173ebec8bebc52f8b9a95b755fd94e3"><span class="icon"></span>0ef7b9e</a></td><td><code>Trac 12121: incremental rounding</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=60759c5ee029464032103a0d04fa3915accbaa36"><span class="icon"></span>60759c5</a></td><td><code>Trac 12121: use incremental_rounding in floor/ceil/round</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=e0396d869f835899ca6d5a75dbe62a8f8dce7413"><span class="icon"></span>e0396d8</a></td><td><code>Trac 12121: implement (broken) floor/ceil for expression</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=83c025750342ffbc079e5d8c0c004d2f1b443f22"><span class="icon"></span>83c0257</a></td><td><code>Trac 12121: fix frac</code>
</td></tr></table>
TicketvdelecroixSat, 17 Sep 2016 13:53:18 GMTstatus changed
https://trac.sagemath.org/ticket/12121#comment:70
https://trac.sagemath.org/ticket/12121#comment:70
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketrwsSat, 17 Sep 2016 14:14:01 GMT
https://trac.sagemath.org/ticket/12121#comment:71
https://trac.sagemath.org/ticket/12121#comment:71
<p>
You removed the symbolic property of <code>frac()</code> and you didn't give any justification for it.
</p>
TicketrwsSat, 17 Sep 2016 14:24:16 GMTstatus changed
https://trac.sagemath.org/ticket/12121#comment:72
https://trac.sagemath.org/ticket/12121#comment:72
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Please try to fix your code so previous frac doctests work.
</p>
TicketgitSat, 17 Sep 2016 16:19:27 GMTcommit changed
https://trac.sagemath.org/ticket/12121#comment:73
https://trac.sagemath.org/ticket/12121#comment:73
<ul>
<li><strong>commit</strong>
changed from <em>83c025750342ffbc079e5d8c0c004d2f1b443f22</em> to <em>5713b9187762d6cd6512936bb6f3ae8674c75da1</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=5713b9187762d6cd6512936bb6f3ae8674c75da1"><span class="icon"></span>5713b91</a></td><td><code>Trac 12121: fix frac</code>
</td></tr></table>
TicketvdelecroixSat, 17 Sep 2016 16:24:05 GMT
https://trac.sagemath.org/ticket/12121#comment:74
https://trac.sagemath.org/ticket/12121#comment:74
<p>
Ralf, what do you call the "symbolic property" of <code>frac</code>?
</p>
<p>
On the other hand I have comments about the previous code of <code>frac</code>
</p>
<ul><li>it is the role of the function <code>floor</code> to call the method <code>floor</code> if needed. There is no need to redo it now and then
</li><li>special casing <code>int</code> and <code>float</code> when the generic <code>x - floor(x)</code> does work is weird (not mentioning that this was not tested). In this case I would prefer that it behaves like floor and ceil (ie frac(float) returning float). The previous version returned Sage integer, why is that?
</li></ul><p>
I let the <code>frac(x + y)</code> not be transformed in <code>x + y - floor(x + y)</code> in my new last commit. Please tell me if you like it better.
</p>
TicketvdelecroixSat, 17 Sep 2016 16:24:15 GMTstatus changed
https://trac.sagemath.org/ticket/12121#comment:75
https://trac.sagemath.org/ticket/12121#comment:75
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketmmezzarobbaSat, 17 Sep 2016 17:04:42 GMT
https://trac.sagemath.org/ticket/12121#comment:76
https://trac.sagemath.org/ticket/12121#comment:76
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:66" title="Comment 66">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
And <code>is_trivial_zero</code> could not be a solution anyway
</p>
<pre class="wiki">sage: delta = (11/9*sqrt(3)*sqrt(2) + 3)^(1/3) + 1/3/(11/9*sqrt(3)*sqrt(2) + 3)^(1/3) - 2
</pre></blockquote>
<p>
Well, of course there will always be examples where the zero-test fails! Above I suggested to try both simplification followed by <code>is_trivial_zero()</code> and conversion to <code>QQbar</code>, this would take care of this example.
</p>
TicketmmezzarobbaSat, 17 Sep 2016 17:07:11 GMT
https://trac.sagemath.org/ticket/12121#comment:77
https://trac.sagemath.org/ticket/12121#comment:77
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:67" title="Comment 67">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
Even better
</p>
<pre class="wiki">sage: (sin(1 - 10^(-100)) - sin(1)).is_zero()
True
age: bool(sin(1 - 10^(-100)) - sin(1) == 0)
True
</pre></blockquote>
<p>
Yes; I thought that was what the comment about <code>is_zero()</code> being unreliable was about.
</p>
TicketvdelecroixSat, 17 Sep 2016 19:03:59 GMT
https://trac.sagemath.org/ticket/12121#comment:78
https://trac.sagemath.org/ticket/12121#comment:78
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:77" title="Comment 77">mmezzarobba</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:67" title="Comment 67">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
Even better
</p>
<pre class="wiki">sage: (sin(1 - 10^(-100)) - sin(1)).is_zero()
True
age: bool(sin(1 - 10^(-100)) - sin(1) == 0)
True
</pre></blockquote>
<p>
Yes; I thought that was what the comment about <code>is_zero()</code> being unreliable was about.
</p>
</blockquote>
<p>
For me unreliable was "sometimes there are false negative". But it is not only that as there are "false positive". Meaning that
</p>
<pre class="wiki">def is_zero(x):
return randint(0, 1)
</pre><p>
is equally good.
</p>
TicketvdelecroixSat, 17 Sep 2016 19:04:34 GMT
https://trac.sagemath.org/ticket/12121#comment:79
https://trac.sagemath.org/ticket/12121#comment:79
<p>
If you have a <code>reliable_is_zero_for_SR</code> I will of course include it ;-)
</p>
TicketrwsSun, 18 Sep 2016 06:18:29 GMT
https://trac.sagemath.org/ticket/12121#comment:80
https://trac.sagemath.org/ticket/12121#comment:80
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:74" title="Comment 74">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
Ralf, what do you call the "symbolic property" of <code>frac</code>?
</p>
</blockquote>
<p>
That it can be part of expressions. In the first version of your "fix frac" commit you unconditionally expanded <code>frac(...)</code> and forced the user to use <code>hold=True</code> to get the symbolic <code>frac()</code>.
</p>
<blockquote class="citation">
<p>
I let the <code>frac(x + y)</code> not be transformed in <code>x + y - floor(x + y)</code> in my new last commit. Please tell me if you like it better.
</p>
</blockquote>
<p>
I do!
</p>
<p>
Marc:
</p>
<blockquote class="citation">
<p>
Well, of course there will always be examples where the zero-test fails! Above I suggested to try both simplification followed by is_trivial_zero() and conversion to QQbar, this would take care of this example.
</p>
</blockquote>
<p>
In <a class="closed ticket" href="https://trac.sagemath.org/ticket/16397" title="defect: Symbolic cmp (closed: fixed)">#16397</a> I implemented this already in <a class="ext-link" href="https://github.com/sagemath/sage/blob/master/src/sage/symbolic/comparison.pyx#L291"><span class="icon"></span>https://github.com/sagemath/sage/blob/master/src/sage/symbolic/comparison.pyx#L291</a> to have some code usable for <code>__nonzero__</code> later.
</p>
TicketmmezzarobbaTue, 20 Sep 2016 07:50:31 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/12121#comment:81
https://trac.sagemath.org/ticket/12121#comment:81
<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>test failures</em>
</li>
</ul>
TicketmmezzarobbaTue, 20 Sep 2016 08:01:09 GMT
https://trac.sagemath.org/ticket/12121#comment:82
https://trac.sagemath.org/ticket/12121#comment:82
<p>
A minor suggestion: perhaps make <code>rounding()</code> to <code>_rounding()</code>?
</p>
<p>
Also, I'm not sure how bad the existing implementation of <code>floor()</code> and friends is, but unless it is really terrible, I'd prefer to either avoid relying on <code>is_zero()</code> (even with known bugs marked as such) or to have <code>is_zero()</code> fixed before merging this ticket.
</p>
TicketmmezzarobbaTue, 20 Sep 2016 08:18:48 GMT
https://trac.sagemath.org/ticket/12121#comment:83
https://trac.sagemath.org/ticket/12121#comment:83
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12121#comment:80" title="Comment 80">rws</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Well, of course there will always be examples where the zero-test fails! Above I suggested to try both simplification followed by is_trivial_zero() and conversion to QQbar, this would take care of this example.
</p>
</blockquote>
<p>
In <a class="closed ticket" href="https://trac.sagemath.org/ticket/16397" title="defect: Symbolic cmp (closed: fixed)">#16397</a> I implemented this already in <a class="ext-link" href="https://github.com/sagemath/sage/blob/master/src/sage/symbolic/comparison.pyx#L291"><span class="icon"></span>https://github.com/sagemath/sage/blob/master/src/sage/symbolic/comparison.pyx#L291</a> to have some code usable for <code>__nonzero__</code> later.
</p>
</blockquote>
<p>
I think I don't follow you, sorry. The code you link to looks like it is intended to sort expressions for printing etc., not to provide reliable mathematical results, isn't it? Besides (but this is starting to be off-topic for this ticket), I'm tempted to think that, for non-relational expressions at least, <code>__nonzero__()</code> should simply be the negation of <code>is_trivial_zero()</code>. As far as I understand, what <code>__nonzero__()</code> is intended to test is whether something is “empty”, “trivial”; it should be as fast as possible, and there is no expectation that it tries hard to prove the nullity of its argument. For a “mathematical” example, I'd find it entirely reasonable to have <code>(x - x)*y + 1 ∈ SR[y]</code> be considered a polynomial of degree <strong>one</strong>—and that's the kind of things <code>__nonzero__()</code> is for. I'm less certain about <em>relational</em> expressions: perhaps <code>bool(expr == 0)</code>, unlike <code>bool(expr)</code>, should keep trying hard to show that <code>expr</code> is zero.
</p>
TicketpelegmSun, 04 Dec 2016 19:08:51 GMTcc changed
https://trac.sagemath.org/ticket/12121#comment:84
https://trac.sagemath.org/ticket/12121#comment:84
<ul>
<li><strong>cc</strong>
<em>pelegm</em> added
</li>
</ul>
TicketmmezzarobbaMon, 19 Dec 2016 19:58:30 GMT
https://trac.sagemath.org/ticket/12121#comment:85
https://trac.sagemath.org/ticket/12121#comment:85
<p>
See also <a class="closed ticket" href="https://trac.sagemath.org/ticket/22079" title="defect: New implementation of floor()/ceil() (closed: fixed)">#22079</a>.
</p>
TicketmmezzarobbaMon, 04 Sep 2017 14:23:33 GMTcommit, branch changed
https://trac.sagemath.org/ticket/12121#comment:86
https://trac.sagemath.org/ticket/12121#comment:86
<ul>
<li><strong>commit</strong>
changed from <em>5713b9187762d6cd6512936bb6f3ae8674c75da1</em> to <em>1b1a32abd788a451a7004c833f03c7a94f86753f</em>
</li>
<li><strong>branch</strong>
changed from <em>u/vdelecroix/12121</em> to <em>u/mmezzarobba/ticket/12121-rebased</em>
</li>
</ul>
<p>
Rebased following the discussion at <a class="closed ticket" href="https://trac.sagemath.org/ticket/22079" title="defect: New implementation of floor()/ceil() (closed: fixed)">#22079</a>.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=1dc641831391e664f7a35940c9c3c37d46a8552a"><span class="icon"></span>1dc6418</a></td><td><code>Trac 12121: incremental rounding</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=4669e113d991290c6a98ae9229db7df8dfe26f48"><span class="icon"></span>4669e11</a></td><td><code>Trac 12121: use incremental_rounding in floor/ceil/round</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=00959ea2b73b8259935bb826f14c3114d2ba042f"><span class="icon"></span>00959ea</a></td><td><code>Trac 12121: implement (broken) floor/ceil for expression</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=1b1a32abd788a451a7004c833f03c7a94f86753f"><span class="icon"></span>1b1a32a</a></td><td><code>Trac 12121: fix frac</code>
</td></tr></table>
TicketjdemeyerMon, 04 Sep 2017 15:04:35 GMT
https://trac.sagemath.org/ticket/12121#comment:87
https://trac.sagemath.org/ticket/12121#comment:87
<p>
I have an implementation fixing <code>floor()</code>/<code>ceil()</code> at <a class="closed ticket" href="https://trac.sagemath.org/ticket/22079" title="defect: New implementation of floor()/ceil() (closed: fixed)">#22079</a>.
</p>
TicketjdemeyerWed, 29 Nov 2017 15:53:32 GMT
https://trac.sagemath.org/ticket/12121#comment:88
https://trac.sagemath.org/ticket/12121#comment:88
<p>
This branch does a whole lot more than fixing <code>floor()</code>/<code>ceil()</code>. Now that this has been fixed in <a class="closed ticket" href="https://trac.sagemath.org/ticket/22079" title="defect: New implementation of floor()/ceil() (closed: fixed)">#22079</a>, feel free to change the purpose of this ticket.
</p>
Ticket