Opened 5 weeks ago

Closed 3 weeks ago

#27049 closed enhancement (fixed)

py3: some fix in Riemann surfaces

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.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) Commit: f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca
Dependencies: Stopgaps:

Description

only partial, there remains 4 failing doctests out of 36

Change History (26)

comment:1 Changed 5 weeks ago by chapoton

  • Branch set to u/chapoton/27049
  • Commit set to 31bceafa0eff35a8ad6a2d701c155794af5ecfa8
  • Status changed from new to needs_review

New commits:

31bceafpy3 some partial fix for Riemann surfaces

comment:2 Changed 5 weeks ago by chapoton

  • Cc tscrim added

green bot, please review

comment:3 Changed 5 weeks ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:4 Changed 5 weeks ago by nbruin

  • Branch changed from u/chapoton/27049 to u/nbruin/27049

comment:5 Changed 5 weeks ago by nbruin

  • 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:

9176ecbUse sqrt method, which is faster and more specific, rather than generic function
Last edited 5 weeks ago by nbruin (previous) (diff)

comment:6 Changed 5 weeks ago by nbruin

  • 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 follow-up: Changed 5 weeks ago by tscrim

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Nils Bruin
  • 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 5 weeks ago by chapoton

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.

EDIT: pep8 is official for sage, see

https://doc.sagemath.org/html/en/developer/coding_basics.html#python-code-style

Last edited 5 weeks ago by chapoton (previous) (diff)

comment:9 in reply to: ↑ 7 Changed 5 weeks ago by nbruin

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 follow-up: Changed 5 weeks ago by chapoton

Maybe this python3-like behaviour is caused by the line

from __future__ import division

that we really want to add.

comment:11 in reply to: ↑ 10 Changed 5 weeks ago by nbruin

Replying to chapoton:

Maybe this python3-like behaviour is caused by the line

from __future__ import division

that 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 follow-up: Changed 5 weeks ago by chapoton

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 ; follow-up: Changed 5 weeks ago by nbruin

Replying to chapoton:

We could try to use **QQ((1,2))but it will probably be just as slow as sqrt(...)

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 5 weeks ago by git

  • Commit changed from 9176ecb0378e1329e39cc13164b01b5a6eca22b3 to 0c4d65bc17d3d406ca1f225a27ef49055edc9140

Branch pushed to git repo; I updated commit sha1. New commits:

0c4d65btry to use shift instead of division-by-two, to work around possibly issues with future division. It might be faster too.

comment:15 Changed 5 weeks ago by nbruin

Let's see how the bot fares with this.

comment:16 Changed 5 weeks ago by chapoton

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/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'

comment:17 in reply to: ↑ 13 Changed 5 weeks ago by chapoton

Did you try with a python3-built sage ?

Replying to nbruin:

Replying to chapoton:

We could try to use **QQ((1,2))but it will probably be just as slow as sqrt(...)

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 5 weeks ago by chapoton

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 5 weeks ago by git

  • Commit changed from 0c4d65bc17d3d406ca1f225a27ef49055edc9140 to f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f6b68b9Use nth_root to take a root (not a fractional exponent)

comment:20 Changed 5 weeks ago by nbruin

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 5 weeks ago by chapoton

ok, thanks. This seems to work both in python2 and in python3. I have launched my bot on it.

comment:22 Changed 5 weeks ago by chapoton

  • Status changed from needs_work to needs_review

comment:23 Changed 5 weeks ago by chapoton

  • Milestone changed from sage-8.6 to sage-8.7

comment:24 Changed 4 weeks ago by chapoton

Bot is now green. so this should be good to go. Positive review, everybody ?

comment:25 Changed 4 weeks ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:26 Changed 3 weeks ago by vbraun

  • Branch changed from u/nbruin/27049 to f6b68b9ad9b814bc20cdec7275ebfd4375e8f1ca
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.