Opened 2 years ago
Closed 2 years ago
#27049 closed enhancement (fixed)
py3: some fix in Riemann surfaces
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  python3  Keywords:  
Cc:  tscrim  Merged in:  
Authors:  Frédéric Chapoton, Nils Bruin  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  f6b68b9 (Commits, GitHub, GitLab)  Commit:  f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca 
Dependencies:  Stopgaps: 
Description
only partial, there remains 4 failing doctests out of 36
Change History (26)
comment:1 Changed 2 years ago by
 Branch set to u/chapoton/27049
 Commit set to 31bceafa0eff35a8ad6a2d701c155794af5ecfa8
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:4 Changed 2 years ago by
 Branch changed from u/chapoton/27049 to u/nbruin/27049
comment:5 Changed 2 years ago by
 Commit changed from 31bceafa0eff35a8ad6a2d701c155794af5ecfa8 to 9176ecb0378e1329e39cc13164b01b5a6eca22b3
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.
(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 :) )
New commits:
9176ecb  Use sqrt method, which is faster and more specific, rather than generic function

comment:6 Changed 2 years ago by
 Status changed from positive_review to needs_review
Very minor change, but I guess someone should take a look at it before we set it back to positive review.
comment:7 followup: ↓ 9 Changed 2 years ago by
 Status changed from needs_review to needs_work
It would be fine with me except it can have a float
input, which leads to these doctest failures (according to the patchbot):
sage t long src/sage/schemes/riemann_surfaces/riemann_surface.py # 32 doctests failed
comment:8 Changed 2 years ago by
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.
comment:9 in reply to: ↑ 7 Changed 2 years ago by
Replying to tscrim:
It would be fine with me except it can have a
float
input, which leads to these doctest failures (according to the patchbot)
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.
comment:10 followup: ↓ 11 Changed 2 years ago by
Maybe this python3like behaviour is caused by the line
from __future__ import division
that we really want to add.
comment:11 in reply to: ↑ 10 Changed 2 years ago by
Replying to chapoton:
Maybe this python3like behaviour is caused by the line
from __future__ import divisionthat we really want to add.
Apparently only on some platforms then, because I ran this exact branch without problems. It means some divisions need to be encoded differently.
Is it indeed the case that dividing an element of RealField? by a python integer may yield a float on some and a RealField? element on others if "division" is imported? That would be really bad and would make porting a lot harder.
comment:12 followup: ↓ 13 Changed 2 years ago by
I don't know.
We could try to use **QQ((1,2))
but it will probably be just as slow as sqrt(...)
comment:13 in reply to: ↑ 12 ; followup: ↓ 17 Changed 2 years ago by
Replying to chapoton:
We could try to use
**QQ((1,2))
but it will probably be just as slow assqrt(...)
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.
comment:14 Changed 2 years ago by
 Commit changed from 9176ecb0378e1329e39cc13164b01b5a6eca22b3 to 0c4d65bc17d3d406ca1f225a27ef49055edc9140
Branch pushed to git repo; I updated commit sha1. New commits:
0c4d65b  try to use shift instead of divisionbytwo, to work around possibly issues with future division. It might be faster too.

comment:15 Changed 2 years ago by
Let's see how the bot fares with this.
comment:16 Changed 2 years ago by
got with python2 77 doctests failings in the modified file, for example
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/sitepackages/sage/doctest/forker.py", line 671, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/chapoton/sage/local/lib/python2.7/sitepackages/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/sitepackages/sage/schemes/riemann_surfaces/riemann_surface.py", line 2043, in __add__ return RiemannSurfaceSum([self,other]) File "/home/chapoton/sage/local/lib/python2.7/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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'
comment:17 in reply to: ↑ 13 Changed 2 years ago by
Did you try with a python3built sage ?
Replying to nbruin:
Replying to chapoton:
We could try to use
**QQ((1,2))
but it will probably be just as slow assqrt(...)
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.
comment:18 Changed 2 years ago by
When undoing the bad change in lowerbound = self._CC(self._a0) >> 1
,
I still get 36 failing doctests in python3 and 32 failing doctests in python2 (same as the one reported by Travis above)
comment:19 Changed 2 years ago by
 Commit changed from 0c4d65bc17d3d406ca1f225a27ef49055edc9140 to f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f6b68b9  Use nth_root to take a root (not a fractional exponent)

comment:20 Changed 2 years ago by
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.
Hopefully the problem is solved now.
comment:21 Changed 2 years ago by
ok, thanks. This seems to work both in python2 and in python3. I have launched my bot on it.
comment:22 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:23 Changed 2 years ago by
 Milestone changed from sage8.6 to sage8.7
comment:24 Changed 2 years ago by
Bot is now green. so this should be good to go. Positive review, everybody ?
comment:26 Changed 2 years ago by
 Branch changed from u/nbruin/27049 to f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3 some partial fix for Riemann surfaces