[with patch; positive review] implement computing manin constants of elliptic curves

[cremona]

Logic looks ok, but patch does not apply to 3.3.alpha2 cleanly. Does it need alpha3? If so a proper review will have to wait (at least until I get home).

Have you remembered 990h where the optimal curve is not #1?

[was]

Have you remembered 990h where the optimal curve is not #1?

No, I forgot about that. Is that the only example? I will add a new command optimal_curve() that finds the optimal curve using your database and includes a workaround for 990h (and any other examples).

Logic looks ok, but patch does not apply to 3.3.alpha2 cleanly. Does it need alpha3? If so a proper review will have to wait

(at least until I get home).

I did it against alpha0. I'll rebase it for alpha3.

[cremona]

Replying to was:

Have you remembered 990h where the optimal curve is not #1?

No, I forgot about that. Is that the only example? I will add a new command optimal_curve() that finds the optimal curve using your database and includes a workaround for 990h (and any other examples).

That's the only example, luckily.

Logic looks ok, but patch does not apply to 3.3.alpha2 cleanly. Does it need alpha3? If so a proper review will have to wait

(at least until I get home).

I did it against alpha0. I'll rebase it for alpha3.

OK, I'll look at it again over the weekend.

[was]

Thanks for pointing out the 990h issue which I had forgot. I found a bug related to that (but not this ticket) and posted a fix at #5149.

[was]

The attached patch implements computation of the Manin constant with some caveats that are clearly spelled out in the docstrings. Also, it fixes a serious bug in an internal function (_multiple_of_degree_of_isogeny_to_optimal_curve, which was just nonsense before).

[cremona]

Patch applies cleanly to 3.3.alpha2 + #5139. Tests pass BUT:

sage: Ellsage: EllipticCurve('990a1').optimal_curve()
---------------------------------------------------------------------------
RuntimeError          

since on line 3099 you set the number to 3 when N=990 for all isogeny classes, not just class h.

Somewhere in the database code I think we have utilities for splitting the label into its 3 components, by the way, which might be more transparent than (e.g.) stripping off the last character to get the number.

[was]

Thanks John -- excellent catch. And, I changed the code to use the code from database/cremona, as you suggested.

[cremona]

Looks good!

[mabshoff]

Merged both patches in Sage 3.3.alpha4.

Cheers,

Michael