upgrade eclib to v20190226

[JohnCremona]

Release v20170815 introduced a modular symbol scaling bug (while sorting out modular symbol scaling essentially correctly) in functions not currently used by Sage. Other releases since the current one in Sage (v20180815) make some interface changes including precision handling (now using bits not digits), so some Sage code changes are likely to be necessary.

Updated source: http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20190226.tar.bz2

CC: @slel @kiwifb @timokau

Component: packages: standard

Keywords: eclib, upgrade

Author: John Cremona, Jeroen Demeyer

Branch: 901eaf8

Reviewer: Jeroen Demeyer, John Cremona

Issue created by migration from https://trac.sagemath.org/ticket/27360

[slel]

Changed keywords from eclib to eclib, upgrade

[JohnCremona]

comment:2

Thanks for adding the new keyword. I have built 8.7.beta5 and am starting to test.

[JohnCremona]

comment:3

The current tarball's configure script sets compiler flags which result in code requiring an optional FLINT module which Sage's FLINT does not have, which I have on my own machine and always forget to remove for Sage. I will replace it. (The module is hmod_mat which allows for linear algebra mod p using 32-bit integers.)

[JohnCremona]

New commits:

25d6db5

#27360 eclib upgrade to v20190226

[JohnCremona]

Commit: 25d6db5

[JohnCremona]

Branch: u/cremona/eclib

[JohnCremona]

comment:6

I did the testing on a python3 build of Sage.

[jdemeyer]

comment:7

Typo: procision

[JohnCremona]

comment:9

Replying to @JohnCremona:

I did the testing on a python3 build of Sage.

That is what I thought but it seems that the steps make distclean; make build took me back to python2 (where all is well with the patch uploaded).

I will build the branch from scratch again, this time making sure it is python3.

[jdemeyer]

comment:10

Replying to @JohnCremona:

make distclean [..] took me back to python2

Indeed. make distclean is meant to remove everything, including output generated by ./configure indicating that you wanted a Python 3 build.

[JohnCremona]

comment:11

Thanks -- I misunderstood Vincent's recent instructions on sage-devel. So now the situation is that this branch works fine under python2 but not under python3 where the output buffering issue arises. To be clear: after switching to this commit and putting the new source tarball into upstream/ I do

make distclean
./configure --with-python=3
MAKE='make -j20' make
./sage -t src/sage/libs/eclib

and see doctest failures in 2 files all of which (by eye) are caused by output buffering.

The same happens on a different machine and a python3 build of vanilla 8.7.beta5 so this issue is not related to the eclib upgrade at all. For that reason it might be better to open a new ticket for it, and not hold up this one.

[JohnCremona]

comment:12

As a proof that this is the issue, after

 export PYTHONUNBUFFERED=1

the test

./sage -t src/sage/libs/eclib

runs fine. I suppose that we do not want to turn off all python output buffering always?

[jdemeyer]

comment:13

The last comment is surprising to me... how would disabling Python buffering fix buffering issues in eclib?

[jdemeyer]

comment:14

Minor nitpick, but why this change?

-    r"""
-    Set the global NTL real number precision.  This has a massive
+    r""" Set the global NTL real number bit precision.  This has a massive

I don't know if it's right or wrong to format docstrings like that, but in any case this seems to go against the typical Sage docstring style. And if you really insist on starting the text on the line with the """ I would at least remove the leading space before Set the.

[jdemeyer]

comment:15

Is -DNTL_ALL now superfluous? If so, you could remove the line

# distutils: extra_compile_args = -DNTL_ALL

from src/sage/libs/eclib/__init__.pxd

[JohnCremona]

comment:16

Replying to @jdemeyer:

The last comment is surprising to me... how would disabling Python buffering fix buffering issues in eclib?

Well if you ignore my "proof" remark it seems to be a fact that setting that environment variable fixes the issue. Nothing has changed in eclib regarding buffering.

One fix would be to make changes in the Sage interface so that whenever an eclib function is called which outputs to stdout, the python stdout is flushed first. It would be possible to add flushing functions in eclib itself but that does not seem the right approach to me.

[JohnCremona]

comment:17

Replying to @jdemeyer:

Is -DNTL_ALL now superfluous? If so, you could remove the line

# distutils: extra_compile_args = -DNTL_ALL

from src/sage/libs/eclib/__init__.pxd

You are right, and I will remove that line.

[JohnCremona]

comment:18

Replying to @jdemeyer:

Minor nitpick, but why this change?

-    r"""
-    Set the global NTL real number precision.  This has a massive
+    r""" Set the global NTL real number bit precision.  This has a massive

I don't know if it's right or wrong to format docstrings like that, but in any case this seems to go against the typical Sage docstring style. And if you really insist on starting the text on the line with the """ I would at least remove the leading space before Set the.

Sorry that is an artefact introduced by emacs python mode which I usually change back manually. I'll do that.

[sagetrac-git]

Changed commit from 25d6db5 to 8c0f976

[sagetrac-git]

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

8c0f976

#27360 changes addressing reviewer suggestions

[jdemeyer]

comment:20

I created #27383 for the buffering issues.

[jdemeyer]

comment:21

There is still procision

[sagetrac-git]

Changed commit from 8c0f976 to 36bf090

[sagetrac-git]

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

36bf090

fix typo

[JohnCremona]

comment:23

Thanks -- I fixed the typo and assume that the buffering questions will be fixed in #27383.

[sagetrac-git]

Changed commit from 3ff19c1 to 8936611

[jdemeyer]

comment:59

This should work now.

[JohnCremona]

comment:60

What is the difference between the commits in comment 56 and those in comment 45?

[jdemeyer]

comment:61

Replying to @JohnCremona:

What is the difference between the commits in comment 56 and those in comment 45?

I just rebased to 8.7.beta7 (which includes #27389)

[JohnCremona]

comment:62

So should I rebuild my python3 using your rebase and test again what I saw at comment 53? Or should we leave this to be dealt with later when other python3 issues are finally fixed?

[jdemeyer]

comment:63

I built and tested this on Python 3 myself. I can indeed reproduce the failure from [comment:52]. I'll try to figure out why it's happening, if only because I'm honestly curious.

[jdemeyer]

comment:64

OK, the difference between Python 2 and 3 is that the same numbers are printed differently. So this suggests a different fix: use %r which always print floats exactly.

[sagetrac-git]

Changed commit from 8936611 to 901eaf8

[sagetrac-git]

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

901eaf8

Print numbers consistently on Python 2 and 3

[JohnCremona]

comment:66

Well spotted, and thanks for fixing. I'm still building the previous version but will continue.

[jdemeyer]

comment:67

I made a few more minor fixes in that function, in particular replacing /= 2 by *= 0.5 (dividing an integer by an integer has different semantics in Python 3, so this might fix a not yet discovered Python 3 bug). I could have written /= 2.0 but decided that the multiplication would be faster too (not that it matters here).

[JohnCremona]

comment:69

When I pull your commit 901eaf8 on top of 8936611 I get a merge conflict in height.py. That seems to be because the parent of your latest commit is not 8936611 but 3ff16c1. Did you mean to get rid of commit 8936611?

[jdemeyer]

comment:70

Don't pull. Just checkout my branch.

[JohnCremona]

comment:71

Replying to @jdemeyer:

Don't pull. Just checkout my branch.

Of course. For some reason I had created a new branch tracking trac/u/jdemeyer/eclib. Still, I think it may be bad practice to upload a branch with same name as an old one but which does not have the old one as parent...

[JohnCremona]

comment:72

OK, so testing all the elliptic_curves directory with a python3 build the only failures I get are ones which have nothing to do with this fix.

For the record there is one failure in each of ell_rational_field, ell_number_field, and padic_lseries. The one in ell_rational_field has been fixed in #27432.

[jdemeyer]

comment:73

Replying to @JohnCremona:

Still, I think it may be bad practice to upload a branch with same name as an old one but which does not have the old one as parent...

I think you're right in principle. On the other hand, sometimes it is more practical to just rebase a branch. For example, having one commit and then another one which reverts that is just adding noise to the git history. In that case, I'd rather just remove the commit.

[JohnCremona]

comment:74

Replying to @jdemeyer:

Replying to @JohnCremona:

Still, I think it may be bad practice to upload a branch with same name as an old one but which does not have the old one as parent...

I think you're right in principle. On the other hand, sometimes it is more practical to just rebase a branch. For example, having one commit and then another one which reverts that is just adding noise to the git history. In that case, I'd rather just remove the commit.

and as a courtesy to others, add a comment on trac to say that that is what you have done!

[vbraun]

Changed branch from u/jdemeyer/eclib to 901eaf8

[jdemeyer]

Changed commit from 901eaf8 to none

[jdemeyer]

comment:76

Good news! This ticket took a bit longer than expected...

[JohnCremona]

comment:77

I would like to publicly thank Jeroen for his work on this. It's not the first time that eclib's inclusion in Sage has led to improvements in the library itself.

The less good news is that some of my code it not converging well with the new precision handling (decimal to bits) but not in a part which affects Sage, so there will not be another ticket to upgrade again. Well, not this month.