Opened 9 years ago

Closed 9 years ago

#14900 closed defect (fixed)

Normalization for modular_symbols is wrong

Reported by: was Owned by: cremona
Priority: minor Milestone: sage-5.12
Component: elliptic curves Keywords:
Cc: wuthrich Merged in: sage-5.12.beta3
Authors: Chris Wuthrich Reviewers: William Stein
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by wuthrich)

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".

Attachments (1)

trac_14900_modsymdoc.patch (4.2 KB) - added by wuthrich 9 years ago.
exported againast 5.10

Download all attachments as: .zip

Change History (6)

comment:1 Changed 9 years ago by cremona

  • Cc wuthrich added

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

Changed 9 years ago by wuthrich

exported againast 5.10

comment:2 Changed 9 years ago by wuthrich

  • Authors set to Chris Wuthrich
  • Description modified (diff)
  • Status changed from new to needs_review

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.

comment:3 Changed 9 years ago by was

  • Status changed from needs_review to positive_review

Looks good to me. Thanks!

comment:4 Changed 9 years ago by jdemeyer

  • Reviewers set to William Stein
  • Summary changed from mistake in docstring for modular_symbols to Normalization for modular_symbols is wrong

comment:5 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.12.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.