Modernize eclib library interface

[jdemeyer]

As part of cleaning up library interfaces, clean up eclib:

1. Move all declarations for eclib library functions to src/sage/libs/eclib/__init__.pxd.

2. Use the C++ capabilities of Cython, in particular use cdef cppclass.

3. Move (with deprecation) the library interface from src/sage/libs/mwrank and src/sage/libs/cremona to src/sage/libs/eclib.

There are no functional changes at all.

Upstream bug found: JohnCremona/eclib#10

CC: @JohnCremona @jpflori

Component: cython

Author: Jeroen Demeyer

Branch/Commit: d3f8404

Reviewer: Jean-Pierre Flori

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

[JohnCremona]

comment:2

I have commented at the upstream Issue, but I would not call this a bug. The build system causes all the code to be compiled with a certain macro defined (namely NTL_ALL), and the code will not build without that flag being defined, but the build system does not allow any way for that macro not to be defined anyway!

[jdemeyer]

comment:3

Replying to @JohnCremona:

I have commented at the upstream Issue, but I would not call this a bug.

It's at least a documentation bug (it's not true that NTL_ALL is the default).

The build system...

Which "build system" are you talking about and why do you claim that "the build system does not allow any way for that macro not to be defined"

[jdemeyer]

comment:4

I am working on cleaning up the Cython interface to eclib and got this doctest failure, any quick idea?

sage -t src/sage/libs/mwrank/interface.py
**********************************************************************
File "src/sage/libs/mwrank/interface.py", line 283, in sage.libs.mwrank.interface.mwrank_EllipticCurve.isogeny_class
Failed example:
    E.isogeny_class()
Expected:
    ([[0, -1, 1, 0, 0], [0, -1, 1, -10, -20], [0, -1, 1, -7820, -263580]], [[0, 5, 0], [5, 0, 5], [0, 5, 0]])
Got:
    ([[0, -1, 1, 0, 0], [0, -1, 1, -10, -20], [0, -1, 1, -7820, -263580]],
     [[0, 0, 5], [0, 0, 0], [5, 0, 0]])
**********************************************************************

[jdemeyer]

comment:5

It seems that simply changing the order of #include "eclib/moddata.h" causes this doctest failure.

[JohnCremona]

comment:7

Replying to @jdemeyer:

Replying to @JohnCremona:

I have commented at the upstream Issue, but I would not call this a bug.

It's at least a documentation bug (it's not true that NTL_ALL is the default).

The build system...

Which "build system" are you talking about and why do you claim that "the build system

I meant the auto tools stuff.

does not allow any way for that macro not to be defined"

There is no option to Configure which affects it.

[JohnCremona]

comment:8

The second output is wrong (the one where the matrix has a zero row ).

moddata.h has nothing at all to do with elliptic curve isogenies!

[jdemeyer]

comment:9

Replying to @JohnCremona:

moddata.h has nothing at all to do with elliptic curve isogenies!

Still, simply including that file earlier breaks that doctest...

[jdemeyer]

Branch: u/jdemeyer/modernize_eclib_library_interface

[jdemeyer]

Commit: 3c83e65

[jdemeyer]

New commits:

3c83e65

Modernize eclib interface

[jdemeyer]

comment:13

@jpflori: could you review this please? It is very similar in spirit to the NTL cleanup we did recently.

[JohnCremona]

comment:14

I agree that changing the name from sage.libs.mwrank is a good idea but why not change it to sage.libs.eclib ?

[jdemeyer]

comment:15

Replying to @JohnCremona:

I agree that changing the name from sage.libs.mwrank is a good idea but why not change it to sage.libs.eclib ?

First of all, tt's clear that all eclib interface code should be in one place, not split in sage.libs.mwrank and sage.libs.cremona.

Do you prefer to change both to sage.libs.eclib?

[JohnCremona]

comment:16

Replying to @jdemeyer:

Replying to @JohnCremona:

I agree that changing the name from sage.libs.mwrank is a good idea but why not change it to sage.libs.eclib ?

First of all, tt's clear that all eclib interface code should be in one place, not split in sage.libs.mwrank and sage.libs.cremona.

Do you prefer to change both to sage.libs.eclib?

Yes I prefer that. eclib might outlive me!

[sagetrac-git]

Changed commit from 3c83e65 to d3f8404

[sagetrac-git]

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

d3f8404

Rename cremona -> eclib

[jpflori]

Reviewer: Jean-Pierre Flori

[jpflori]

comment:20

Looks ok to me.

[vbraun]

Changed branch from u/jdemeyer/modernize_eclib_library_interface to d3f8404