Opened 4 months ago

Closed 4 days ago

#28355 closed enhancement (fixed)

Allow precision to be passed into the elliptic_j function

Reported by: menosgeze Owned by:
Priority: minor Milestone: sage-9.0
Component: elliptic curves Keywords: elliptic_j, precision, elliptic curves
Cc: Merged in:
Authors: Gerardo E Zelaya Eufemia, Kevin Lui Reviewers: Kevin Lui, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 58ba0e8 (Commits) Commit: 58ba0e8ed3f0e007b67b07c3fc9f1c80ca21cffb
Dependencies: Stopgaps:

Description

For example, Wikipedia says that the j-invariant of (1+sqrt(-163))/2 is an integer, but if one computes elliptic_j for this symbolic ring element, it returns a complex number with an imaginary part not close to 0. See:

https://en.wikipedia.org/wiki/J-invariant

sage: b = sqrt(-163)
sage: b = (1+b)/2
sage: elliptic_j(b)
-2.62537412640767e17 + 732.558854258998*I
sage: elliptic_j(ComplexField(100(b))
-2.6253741264076799999999999999e17 + 1.3012822400356887122945119790e-12*I

Change History (25)

comment:1 Changed 4 months ago by menosgeze

  • Branch set to u/menosgeze/allow_precision_to_be_passed_into_the_elliptic_j_function

comment:2 Changed 4 months ago by klui

  • Commit set to 5bfbc61a629982af447ee8e13b7bbdc6b00afe0a

I see that the PARI function ellj also takes in precision. Should we also pass in the precision into that as well?

Style Suggestions:

  • Add extra line before function.
  • Delete spaces around equal sign.
  • Indentation should be a multiple of 4.

Doc string suggestions:

  • Add prec to the input line of documentation.

comment:3 Changed 3 months ago by git

  • Commit changed from 5bfbc61a629982af447ee8e13b7bbdc6b00afe0a to 30092a2a9daa403c8a593d6c4c061b569d45e771

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

30092a2corrected all klui suggestions except pari input precision

comment:4 Changed 3 months ago by git

  • Commit changed from 30092a2a9daa403c8a593d6c4c061b569d45e771 to 1f2495fb7f99fcf039df760f0a79ea46b4a63066

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

1f2495fcorrected trailing spaces

comment:5 Changed 3 months ago by git

  • Commit changed from 1f2495fb7f99fcf039df760f0a79ea46b4a63066 to 0076f6baf548cc0560b18c31dd2fb98aaff7ed71

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

0076f6badded example explanation

comment:6 Changed 3 months ago by git

  • Commit changed from 0076f6baf548cc0560b18c31dd2fb98aaff7ed71 to f4d7ca2bd150ee13179538d59745c2fb80e3182c

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

f4d7ca2corrected explanation of example

comment:7 Changed 3 months ago by git

  • Commit changed from f4d7ca2bd150ee13179538d59745c2fb80e3182c to bd02eb423fbe35ba58d4f0be7984d0d9b02594ad

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

bd02eb4corrected explanation of example

comment:8 Changed 3 months ago by menosgeze

  • Authors set to Gerardo E Zelaya Eufemia
  • Status changed from new to needs_review

The elliptic_j function needed an extra precision argument prec, and I fixed the indentation for the elliptic_j function.


New commits:

bd02eb4corrected explanation of example

comment:9 Changed 3 months ago by klui

  • Status changed from needs_review to needs_work

Docstring formatting error in the input section.

Indentation of L309 is off by one.

comment:10 Changed 3 months ago by git

  • Commit changed from bd02eb423fbe35ba58d4f0be7984d0d9b02594ad to e1751627dd272a3ba54eb549cf5d46d8582d56df

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

e175162corrected line 309 indentation

comment:11 Changed 3 months ago by menosgeze

  • Status changed from needs_work to needs_review

comment:12 Changed 3 months ago by klui

  • Status changed from needs_review to positive_review

Looks good to me now. I think this is a useful change.

I think the current patchbot failures aren't relevant here. The tests pass on my machine.

comment:13 Changed 3 months ago by chapoton

reviewer name is missing

comment:14 Changed 3 months ago by klui

  • Reviewers set to Kevin Lui

Opps. Thanks!

comment:15 Changed 3 months ago by chapoton

  • Milestone changed from sage-8.9 to sage-9.0

moving milestone to 9.0 (after release of 8.9)

comment:16 Changed 2 months ago by vbraun

  • Status changed from positive_review to needs_work

On Ubuntu 10 32-bit:

**********************************************************************
File "src/sage/functions/special.py", line 344, in sage.functions.special.elliptic_j
Failed example:
    -elliptic_j(tau)
Expected:
    2.62537412640767e17 - 732.558854258998*I
Got:
    2.62537412640767e17 - 732.558854103088*I
**********************************************************************
File "src/sage/functions/special.py", line 346, in sage.functions.special.elliptic_j
Failed example:
    -elliptic_j(tau,75)
Expected:
    2.625374126407680000000e17 - 0.0001309913593909879441262*I
Got:
    2.625374126407680000000e17 - 0.0001309913596969636273570*I
**********************************************************************
File "src/sage/functions/special.py", line 348, in sage.functions.special.elliptic_j
Failed example:
    -elliptic_j(tau,100)
Expected:
    2.6253741264076799999999999999e17 - 1.3012822400356887122945119790e-12*I
Got:
    2.6253741264076799999999999999e17 - 1.3012822398031636140364186716e-12*I
**********************************************************************
1 item had failures:
   3 of  12 in sage.functions.special.elliptic_j
    [125 tests, 3 failures, 3.96 s]
----------------------------------------------------------------------
sage -t --long src/sage/functions/special.py  # 3 doctests failed

comment:17 Changed 2 months ago by klui

  • Branch changed from u/menosgeze/allow_precision_to_be_passed_into_the_elliptic_j_function to u/klui/allow_precision_to_be_passed_into_the_elliptic_j_function

comment:18 Changed 2 months ago by klui

  • Commit changed from e1751627dd272a3ba54eb549cf5d46d8582d56df to 843998895cf79a74bd4c087cac221204a8bf7202
  • Status changed from needs_work to needs_review

I did a rebase and I added tolerance for the fixing numerical doctests on 32-bit machine. I don't have access to a 32-bit machine so let's see what the patchbot say.

I'm not too sure about what to do with the ticket fields. Do I remove myself from reviewer and add myself to author? Just leave it as it?


New commits:

0669ee8added precision argument to elliptic_j
49b80b6corrected all klui suggestions except pari input precision
5f1348ccorrected trailing spaces
70870f1added example explanation
d8593a2corrected explanation of example
9371454corrected explanation of example
8a813abcorrected line 309 indentation
8439988Add a rel tol to pass failing numerical doctests on 32-bit machines

comment:19 Changed 4 weeks ago by chapoton

sage -t --long src/sage/functions/special.py  # 3 doctests failed

The tag "rel tol" should be on the command line in the doctest, not on the result line.

You can add yourself as author and stay also reviewer. You need someone else to review your own changes.

comment:20 Changed 4 weeks ago by chapoton

  • Status changed from needs_review to needs_work

comment:21 Changed 3 weeks ago by klui

  • Branch changed from u/klui/allow_precision_to_be_passed_into_the_elliptic_j_function to u/klui/allow_precision_to_be_passed_into_the_elliptic_j_function_rebase
  • Commit 843998895cf79a74bd4c087cac221204a8bf7202 deleted

comment:22 Changed 3 weeks ago by git

  • Commit set to 58ba0e8ed3f0e007b67b07c3fc9f1c80ca21cffb

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

e7f6676added precision argument to elliptic_j
2c20be9corrected all klui suggestions except pari input precision
da83b87corrected trailing spaces
fbb4f83added example explanation
ec1d6f9corrected explanation of example
6cf6390corrected explanation of example
7327263corrected line 309 indentation
5a871a9Add a rel tol to pass failing numerical doctests on 32-bit machines
58ba0e8Fix doctest location of rel/abs tol flag

comment:23 Changed 3 weeks ago by klui

  • Authors changed from Gerardo E Zelaya Eufemia to Gerardo E Zelaya Eufemia, Kevin Lui
  • Status changed from needs_work to needs_review

I did a rebase on develop followed by fixing the location of the tol flags.

comment:24 Changed 5 days ago by chapoton

  • Reviewers changed from Kevin Lui to Kevin Lui, Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok

comment:25 Changed 4 days ago by vbraun

  • Branch changed from u/klui/allow_precision_to_be_passed_into_the_elliptic_j_function_rebase to 58ba0e8ed3f0e007b67b07c3fc9f1c80ca21cffb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.