Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalization for modular_symbols is wrong #14900

Closed
williamstein opened this issue Jul 16, 2013 · 8 comments
Closed

Normalization for modular_symbols is wrong #14900

williamstein opened this issue Jul 16, 2013 · 8 comments

Comments

@williamstein
Copy link
Contributor

The docstring for E.modular_symbol accidentally includes "normalize" twice. I think the first instance, which a user might read first, is wrong and should be deleted (it must have been accidentally left there).

sage: E = EllipticCurve('11a')
sage: E.modular_symbol?
...
        -  ``normalize`` - (default: True); if True, the
           modular symbol is correctly normalized (up to possibly a factor of
           -1 or 2). If False, the modular symbol is almost certainly not
           correctly normalized, i.e., all values will be a fixed scalar
           multiple of what they should be. But the initial computation of the
           modular symbol is much faster, though evaluation of it after
           computing it won't be any faster.
           
        -  ``use_eclib`` - (default: False); if True the computation is
           done with John Cremona's implementation of modular
           symbols in ``eclib``
                
        -  ``normalize`` - (default: 'L_ratio'); either 'L_ratio', 'period', or 'none';
           For 'L_ratio', the modular symbol is correctly normalized
           as explained above by comparing it to ``L_ratio`` for the
           curve and some small twists.
           The normalization 'period' is only available if
...

Apply the patch "trac_14900_modsymdoc.patch".

CC: @categorie

Component: elliptic curves

Author: Chris Wuthrich

Reviewer: William Stein

Merged: sage-5.12.beta3

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

@JohnCremona
Copy link
Member

comment:1

I think you are correct, including the reason why the old version is there. I hope that Chris Wuthrich will confirm.

@chriswuthrich
Copy link
Contributor

exported againast 5.10

@chriswuthrich

This comment has been minimized.

@chriswuthrich
Copy link
Contributor

Author: Chris Wuthrich

@chriswuthrich
Copy link
Contributor

comment:2

Attachment: trac_14900_modsymdoc.patch.gz

Yes, that is what happened an I am sorry. I submit a quick patch here that fixes this and a small other issue with this normalisation. If L_ratio fails it now falls back to period. The cases where this makes a real difference, like 1369b1 are too long to be included as a doctest.

In any case, I am writing on a new implementation of modular symbols which will change this normalisation business radically anyway.

@williamstein
Copy link
Contributor Author

comment:3

Looks good to me. Thanks!

@jdemeyer
Copy link

jdemeyer commented Aug 7, 2013

Reviewer: William Stein

@jdemeyer jdemeyer changed the title mistake in docstring for modular_symbols Normalization for modular_symbols is wrong Aug 7, 2013
@jdemeyer
Copy link

Merged: sage-5.12.beta3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants