pyflakes cleanup of elliptic curves

[fchapoton]

Aim: that all files in src.sage.schemes.elliptic_curves pass pyflakes, except for all.py.

CC: @categorie

Component: elliptic curves

Author: Frédéric Chapoton, John Cremona

Branch/Commit: 70a2fc2

Reviewer: John Cremona, Frédéric Chapoton

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

[fchapoton]

Branch: public/25463

[fchapoton]

Commit: 3ecf2dc

[fchapoton]

New commits:

3ecf2dc

partial pyflakes cleanup of schemes.elliptic_curves (much needed)

[JohnCremona]

comment:2

I will look at this, and with luck will finish it off. Thanks for doing the bulk so far.

[sagetrac-git]

Changed commit from 3ecf2dc to 596f49f

[sagetrac-git]

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

596f49f

#25463 more pyflakes cleanup for sage.schemes.elliptic_curves

[JohnCremona]

comment:4

OK so I positively review all the changes made in the first commit, and offer more, which almost finishes off sage.schemes.elliptic_curves.

Remaining issues:

1. padic_lseries has 4 lines 1627-30 which look redundant but I want to ask someone else.
2. all.py: this must be a general thing to decide if we want pyflakes not to comlain about almost all all.py files! One way round it is to simply add a line "assert x" for everything imported, but is that necessary?
3. There's a test in ell_number_field which is tagged long time with an estimate (by me probably) of 80s but it now takes 180s which is ridiculous. I don't have time now to track down the regression, but there must be one somewhere. I will open a ticket for that after checking that there is not yet one (I think there is a ticket about slow doctests for that file).

[fchapoton]

comment:5

I think we should already do the changes here, and maybe care for the few remaining ones later in another ticket. There is no need to do anything for all.py.

I agree with your changes, and propose to set to positive once a patchbot is green.

[fchapoton]

comment:6

There are failing doctests in

sage -t src/sage/schemes/elliptic_curves/BSD.py

These 3 failing doctests seem to be improvements over the existing ones. Should we just change them ?

[JohnCremona]

comment:7

Yes, it seems that fixing the typo on current line means that the 5-part of the proof now goes through. I would like Chris Wuthrich to give a view on both this and the one in padic_lseries before we merge this, but I will make an updated commit now.

I guess this will be the first time in history where pyflakes has succeeded in solving one of the Millennium $1M Prize problems!

[JohnCremona]

Reviewer: John Cremona, Frédéric Chapoton

[JohnCremona]

Changed author from Frédéric Chapoton to Frédéric Chapoton, John Cremona

[sagetrac-git]

Changed commit from 596f49f to 2a7282a

[sagetrac-git]

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

2a7282a

#25463: finish pyflaking elliptic_curves

[chriswuthrich]

comment:10

I guess I will have to learn what pyflakes is. By now I assume it is a thing that picks up redundant or bad lines. Tell me if I need more. (Is there a version for cython, too? The amount of cython warnings during compilation should frighten anyone.)

padic_lseries has 4 lines 1627-30 which look redundant but I want to ask someone else.

        if p < 5:
            phi = self.frobenius(min(6,prec),algorithm="approx")
        else:
            phi = self.frobenius(prec+2,algorithm="mw")

Yes, they are, please delete.
phi enters the picture only at line 1737.

Yes, the crit(e)_lw is mine. Sorry about this and thanks for correcting. I agree that the doctest can be changed as proposed.

Let me know if I need to look at anything further, I am happy to help.

[sagetrac-git]

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

70a2fc2

#25463: finish pyflaking elliptic_curves

[sagetrac-git]

Changed commit from 2a7282a to 70a2fc2

[JohnCremona]

comment:12

Thanks -- all the last commit does is delete the lines which had been commented out.

Yes, pyflakes does a syntax check & warns of unused imports, of variables assigned to but never then used (which can be a bug, as with one of these), or re-use of a variable name. I don't know about cython.

[fchapoton]

comment:13

I have just launched my patchbot. We can give a positive review once it's green.

[fchapoton]

comment:14

green bot. Thanks !

[vbraun]

Changed branch from public/25463 to 70a2fc2