Sage: Ticket #27049: py3: some fix in Riemann surfaces
https://trac.sagemath.org/ticket/27049
<p>
only partial, there remains 4 failing doctests out of 36
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/27049
Trac 1.1.6chapotonSun, 13 Jan 2019 09:31:30 GMTstatus changed; commit, branch set
https://trac.sagemath.org/ticket/27049#comment:1
https://trac.sagemath.org/ticket/27049#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>31bceafa0eff35a8ad6a2d701c155794af5ecfa8</em>
</li>
<li><strong>branch</strong>
set to <em>u/chapoton/27049</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=31bceafa0eff35a8ad6a2d701c155794af5ecfa8"><span class="icon"></span>31bceaf</a></td><td><code>py3 some partial fix for Riemann surfaces</code>
</td></tr></table>
TicketchapotonSun, 13 Jan 2019 14:18:53 GMTcc set
https://trac.sagemath.org/ticket/27049#comment:2
https://trac.sagemath.org/ticket/27049#comment:2
<ul>
<li><strong>cc</strong>
<em>tscrim</em> added
</li>
</ul>
<p>
green bot, please review
</p>
TickettscrimSun, 13 Jan 2019 17:01:18 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/27049#comment:3
https://trac.sagemath.org/ticket/27049#comment:3
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Travis Scrimshaw</em>
</li>
</ul>
<p>
LGTM.
</p>
TicketnbruinMon, 14 Jan 2019 20:58:49 GMTbranch changed
https://trac.sagemath.org/ticket/27049#comment:4
https://trac.sagemath.org/ticket/27049#comment:4
<ul>
<li><strong>branch</strong>
changed from <em>u/chapoton/27049</em> to <em>u/nbruin/27049</em>
</li>
</ul>
TicketnbruinMon, 14 Jan 2019 21:09:28 GMTcommit changed
https://trac.sagemath.org/ticket/27049#comment:5
https://trac.sagemath.org/ticket/27049#comment:5
<ul>
<li><strong>commit</strong>
changed from <em>31bceafa0eff35a8ad6a2d701c155794af5ecfa8</em> to <em>9176ecb0378e1329e39cc13164b01b5a6eca22b3</em>
</li>
</ul>
<p>
Changed back to using sqrt method. It's much faster because it doesn't have to do all the generic stuff. This is a core routine -- it actually makes a difference if it's fast.
</p>
<p>
(also -- not a fan of all the trivial whitespace edits. It makes it hard to tell from the diff what has actually changed. If you have to touch a line anyway: sure, go ahead. Otherwise I think it's better to just leave it in the original form and not impose (subjective) aesthetics --PEP8's suggestions are mainly against too much whitespace; less so for too little :-) )
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=9176ecb0378e1329e39cc13164b01b5a6eca22b3"><span class="icon"></span>9176ecb</a></td><td><code>Use sqrt method, which is faster and more specific, rather than generic function</code>
</td></tr></table>
TicketnbruinMon, 14 Jan 2019 21:33:27 GMTstatus changed
https://trac.sagemath.org/ticket/27049#comment:6
https://trac.sagemath.org/ticket/27049#comment:6
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_review</em>
</li>
</ul>
<p>
Very minor change, but I guess someone should take a look at it before we set it back to positive review.
</p>
TickettscrimTue, 15 Jan 2019 06:28:06 GMTstatus, author changed
https://trac.sagemath.org/ticket/27049#comment:7
https://trac.sagemath.org/ticket/27049#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>author</strong>
changed from <em>Frédéric Chapoton</em> to <em>Frédéric Chapoton, Nils Bruin</em>
</li>
</ul>
<p>
It would be fine with me except it can have a <code>float</code> input, which leads to these doctest failures (according to the patchbot):
</p>
<pre class="wiki">sage -t --long src/sage/schemes/riemann_surfaces/riemann_surface.py # 32 doctests failed
</pre>
TicketchapotonTue, 15 Jan 2019 07:22:28 GMT
https://trac.sagemath.org/ticket/27049#comment:8
https://trac.sagemath.org/ticket/27049#comment:8
<p>
Whatever way, the point of this ticket is to make this module's doctests pass with both python2 and python3. If you want to keep the speed, this may not be so easy.
</p>
<p>
EDIT: pep8 is official for sage, see
</p>
<p>
<a class="ext-link" href="https://doc.sagemath.org/html/en/developer/coding_basics.html#python-code-style"><span class="icon"></span>https://doc.sagemath.org/html/en/developer/coding_basics.html#python-code-style</a>
</p>
TicketnbruinTue, 15 Jan 2019 07:26:28 GMT
https://trac.sagemath.org/ticket/27049#comment:9
https://trac.sagemath.org/ticket/27049#comment:9
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/27049#comment:7" title="Comment 7">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It would be fine with me except it can have a <code>float</code> input, which leads to these doctest failures (according to the patchbot)
</p>
</blockquote>
<p>
That's strange. I cannot replicate that behaviour. It seems to me this bot is testing python 2, so it doesn't look like a py2/3 issue. Is this bot anomalous? If I had access to a platform where this behaviour occurs, I could probably debug this quite quickly.
</p>
TicketchapotonTue, 15 Jan 2019 07:30:06 GMT
https://trac.sagemath.org/ticket/27049#comment:10
https://trac.sagemath.org/ticket/27049#comment:10
<p>
Maybe this python3-like behaviour is caused by the line
</p>
<pre class="wiki">from __future__ import division
</pre><p>
that we really want to add.
</p>
TicketnbruinTue, 15 Jan 2019 07:38:47 GMT
https://trac.sagemath.org/ticket/27049#comment:11
https://trac.sagemath.org/ticket/27049#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/27049#comment:10" title="Comment 10">chapoton</a>:
</p>
<blockquote class="citation">
<p>
Maybe this python3-like behaviour is caused by the line
</p>
<pre class="wiki">from __future__ import division
</pre><p>
that we really want to add.
</p>
</blockquote>
<p>
Apparently only on some platforms then, because I ran this exact branch without problems. It means some divisions need to be encoded differently.
</p>
<p>
Is it indeed the case that dividing an element of <a class="missing wiki">RealField?</a> by a python integer may yield a float on some and a <a class="missing wiki">RealField?</a> element on others if "division" is imported? That would be really bad and would make porting a lot harder.
</p>
TicketchapotonTue, 15 Jan 2019 07:45:54 GMT
https://trac.sagemath.org/ticket/27049#comment:12
https://trac.sagemath.org/ticket/27049#comment:12
<p>
I don't know.
</p>
<p>
We could try to use <code>**QQ((1,2))</code>but it will probably be just as slow as <code>sqrt(...)</code>
</p>
TicketnbruinTue, 15 Jan 2019 08:08:36 GMT
https://trac.sagemath.org/ticket/27049#comment:13
https://trac.sagemath.org/ticket/27049#comment:13
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/27049#comment:12" title="Comment 12">chapoton</a>:
</p>
<blockquote class="citation">
<p>
We could try to use <code>**QQ((1,2))</code>but it will probably be just as slow as <code>sqrt(...)</code>
</p>
</blockquote>
<p>
No, if this really is a problem we can replace the "division by two" by an exponent operation, which would be very fast. I just need to know which operations are the problematic ones. On the computers I have access to, there is no problem, so I can't debug the problem presently.
</p>
TicketgitTue, 15 Jan 2019 08:14:22 GMTcommit changed
https://trac.sagemath.org/ticket/27049#comment:14
https://trac.sagemath.org/ticket/27049#comment:14
<ul>
<li><strong>commit</strong>
changed from <em>9176ecb0378e1329e39cc13164b01b5a6eca22b3</em> to <em>0c4d65bc17d3d406ca1f225a27ef49055edc9140</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=0c4d65bc17d3d406ca1f225a27ef49055edc9140"><span class="icon"></span>0c4d65b</a></td><td><code>try to use shift instead of division-by-two, to work around possibly issues with future division. It might be faster too.</code>
</td></tr></table>
TicketnbruinTue, 15 Jan 2019 08:14:49 GMT
https://trac.sagemath.org/ticket/27049#comment:15
https://trac.sagemath.org/ticket/27049#comment:15
<p>
Let's see how the bot fares with this.
</p>
TicketchapotonTue, 15 Jan 2019 08:18:15 GMT
https://trac.sagemath.org/ticket/27049#comment:16
https://trac.sagemath.org/ticket/27049#comment:16
<p>
got with python2 77 doctests failings in the modified file, for example
</p>
<pre class="wiki">File "src/sage/schemes/riemann_surfaces/riemann_surface.py", line 2244, in sage.schemes.riemann_surfaces.riemann_surface.RiemannSurfaceSum.__add__
Failed example:
S1+S1+S1
Exception raised:
Traceback (most recent call last):
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.schemes.riemann_surfaces.riemann_surface.RiemannSurfaceSum.__add__[3]>", line 1, in <module>
S1+S1+S1
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 2043, in __add__
return RiemannSurfaceSum([self,other])
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 2168, in __init__
PM = s.period_matrix()
File "sage/misc/cachefunc.pyx", line 2314, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12763)
self.cache = f(self._instance)
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 1562, in period_matrix
PM=self.matrix_of_integral_values(differentials)
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 1499, in matrix_of_integral_values
cycles = self.homology_basis()
File "sage/misc/cachefunc.pyx", line 2314, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12763)
self.cache = f(self._instance)
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 1170, in homology_basis
edgesu = self.upstairs_edges()
File "sage/misc/cachefunc.pyx", line 2314, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12763)
self.cache = f(self._instance)
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 935, in upstairs_edges
homotopycont = self.homotopy_continuation(e)
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 711, in homotopy_continuation
delta = self._compute_delta(path(T), epsilon, wvalues=currw)/path_length
File "/home/chapoton/sage/local/lib/python2.7/site-packages/sage/schemes/riemann_surfaces/riemann_surface.py", line 650, in _compute_delta
lowerbound = self._CC(self._a0) >> 1
TypeError: unsupported operand type(s) for >>: 'sage.rings.complex_number.ComplexNumber' and 'int'
</pre>
TicketchapotonTue, 15 Jan 2019 08:21:27 GMT
https://trac.sagemath.org/ticket/27049#comment:17
https://trac.sagemath.org/ticket/27049#comment:17
<p>
Did you try with a python3-built sage ?
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/27049#comment:13" title="Comment 13">nbruin</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/27049#comment:12" title="Comment 12">chapoton</a>:
</p>
<blockquote class="citation">
<p>
We could try to use <code>**QQ((1,2))</code>but it will probably be just as slow as <code>sqrt(...)</code>
</p>
</blockquote>
<p>
No, if this really is a problem we can replace the "division by two" by an exponent operation, which would be very fast. I just need to know which operations are the problematic ones. On the computers I have access to, there is no problem, so I can't debug the problem presently.
</p>
</blockquote>
TicketchapotonTue, 15 Jan 2019 08:24:37 GMT
https://trac.sagemath.org/ticket/27049#comment:18
https://trac.sagemath.org/ticket/27049#comment:18
<p>
When undoing the bad change in <code>lowerbound = self._CC(self._a0) >> 1</code>,
I still get 36 failing doctests in python3 and 32 failing doctests in python2 (same as the one reported by Travis above)
</p>
TicketgitTue, 15 Jan 2019 08:43:37 GMTcommit changed
https://trac.sagemath.org/ticket/27049#comment:19
https://trac.sagemath.org/ticket/27049#comment:19
<ul>
<li><strong>commit</strong>
changed from <em>0c4d65bc17d3d406ca1f225a27ef49055edc9140</em> to <em>f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca</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=f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca"><span class="icon"></span>f6b68b9</a></td><td><code>Use nth_root to take a root (not a fractional exponent)</code>
</td></tr></table>
TicketnbruinTue, 15 Jan 2019 08:47:31 GMT
https://trac.sagemath.org/ticket/27049#comment:20
https://trac.sagemath.org/ticket/27049#comment:20
<p>
By running "sage -b" I was able to replicate the erroneous behaviour :-). Turns out the problem arose from an erroneous "fractional" exponent that was never fractional, so this code was actually not behaving as intended before, but doing so silently.
</p>
<p>
Hopefully the problem is solved now.
</p>
TicketchapotonTue, 15 Jan 2019 09:06:21 GMT
https://trac.sagemath.org/ticket/27049#comment:21
https://trac.sagemath.org/ticket/27049#comment:21
<p>
ok, thanks. This seems to work both in python2 and in python3. I have launched my bot on it.
</p>
TicketchapotonTue, 15 Jan 2019 09:08:16 GMTstatus changed
https://trac.sagemath.org/ticket/27049#comment:22
https://trac.sagemath.org/ticket/27049#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketchapotonTue, 15 Jan 2019 09:08:29 GMTmilestone changed
https://trac.sagemath.org/ticket/27049#comment:23
https://trac.sagemath.org/ticket/27049#comment:23
<ul>
<li><strong>milestone</strong>
changed from <em>sage-8.6</em> to <em>sage-8.7</em>
</li>
</ul>
TicketchapotonTue, 15 Jan 2019 10:55:03 GMT
https://trac.sagemath.org/ticket/27049#comment:24
https://trac.sagemath.org/ticket/27049#comment:24
<p>
Bot is now green. so this should be good to go. Positive review, everybody ?
</p>
TickettscrimTue, 15 Jan 2019 16:08:52 GMTstatus changed
https://trac.sagemath.org/ticket/27049#comment:25
https://trac.sagemath.org/ticket/27049#comment:25
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
LGTM.
</p>
TicketvbraunThu, 24 Jan 2019 18:17:31 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/27049#comment:26
https://trac.sagemath.org/ticket/27049#comment:26
<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>u/nbruin/27049</em> to <em>f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca</em>
</li>
</ul>
Ticket