Opened 2 years ago

Closed 2 years ago

#28645 closed defect (fixed)

Error in precision of Tate elliptic curves over Qp

Reported by: wuthrich Owned by:
Priority: major Milestone: sage-9.0
Component: elliptic curves Keywords: Tate curve
Cc: Merged in:
Authors: Chris Wuthrich Reviewers: Edgar Costa
Report Upstream: N/A Work issues:
Branch: 9c537fb (Commits, GitHub, GitLab) Commit: 9c537fbbeb0f6106cc637d5aa6e1cfff9437dd4f
Dependencies: Stopgaps:

Status badges

Description

The optional precision arguments for Tate's elliptic curves are incorrectly implemented. For many functions, increasing the precision does not increase the precision of the output.

Change History (12)

comment:1 Changed 2 years ago by wuthrich

  • Branch set to u/wuthrich/tate_curve_prec_28645
  • Commit set to ff01d1ca4707b459247a9dd840d808d6b8c42b05

New commits:

ff01d1cprecision for tate curves

comment:2 Changed 2 years ago by wuthrich

  • Status changed from new to needs_review

Here an example of what was wrong

sage: E = EllipticCurve("126a5")
sage: Eq = E.tate_curve(2)
sage: R = Qp(2,30)
sage: u = R(2019)
sage: Eq.parametrisation_onto_original_curve(u, prec=30)
(2^-2 + 2^-1 + 1 + 2 + 2^3 + 2^7 + 2^8 + 2^15 + O(2^18) : 2^-3 + 2^-2 + 2^-1 + 2 + 2^2 + 2^5 + 2^6 + 2^7 + 2^9 + 2^11 + 2^14 + 2^15 + O(2^16) : 1 + O(2^30))

The precision drops to 20. Now fixed returns:

(2^-2 + 2^-1 + 1 + 2 + 2^3 + 2^7 + 2^8 + 2^15 + 2^18 + 2^19 + 2^20 + 2^21 + 2^26 + O(2^28) : 2^-3 + 2^-2 + 2^-1 + 2 + 2^2 + 2^5 + 2^6 + 2^7 + 2^9 + 2^11 + 2^14 + 2^15 + 2^19 + 2^20 + 2^23 + 2^24 + 2^25 + O(2^26) : 1 + O(2^30))

comment:3 Changed 2 years ago by edgarcosta

  • Status changed from needs_review to needs_work

FYI, src/sage/schemes/elliptic_curves/ell_tate_curve.py:233: local variable 'n' is assigned to but never used

comment:4 Changed 2 years ago by git

  • Commit changed from ff01d1ca4707b459247a9dd840d808d6b8c42b05 to 9c537fbbeb0f6106cc637d5aa6e1cfff9437dd4f

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

9c537fbdel one useless var

comment:5 Changed 2 years ago by wuthrich

  • Status changed from needs_work to needs_review

Delete that line.

Version 0, edited 2 years ago by wuthrich (next)

comment:6 Changed 2 years ago by edgarcosta

I'm sorry, but can't fully test this at the moment, so I can't give you examples, but I spotted a couple of issues (not introduced by you), but propagate through the rest:

1- One is always hitting an AttributeError in the try block in def parameter, as it should be calling precision_absolute. Also, the try block is a bit sad, as it should be just triggered by __init__. Perhaps a getattr(self, '_q', None) would make more sense.

2- Shouldn't we be checking for precision_relative? Given that we are always working with relative precision.

3- Shouldn't parameter return a value with the right precision? If not, we might need to work more and get results with more precision than expected.

4- Similar to the previous item, we should consider replacing R = qE.parent() by R = Qp(self._p, prec=prec) in def curve.

comment:7 Changed 2 years ago by wuthrich

You may be right that there are more things that should be improved in that file. The purpose of this ticket is to correct a bug: I can increase the precision of the output by increasing the parameter prec. That was broken.

My proposal is that this is done on this ticket. Then we can open a further ticket to improve things.

In particular, I can get rid of try blocks (this is early sage when there were try blocks everywhere). We could also think about getting provable correct precisions out. Honestly, I have little interest in doing so, but I would be willing to go through the file and make the necessary changes. But I doubt that anyone uses the output in that way. We should at least, give a warning that it is not provable relative/absolute precision.

If we want to do everything on this ticket, it is likely to drag for months and may get abandoned when this simple quick fix would have improved sage.

comment:8 Changed 2 years ago by edgarcosta

I agree with you, you should not fix this now.

Regarding the original bug, is this an expected output, or should one raise an exception?

sage: E = EllipticCurve("126a5")
sage: Eq = E.tate_curve(2)
sage: R = Qp(2,30)
sage: u = R(2019)
sage: Eq.parametrisation_onto_original_curve(u, prec=60)
(2^-2 + 2^-1 + 1 + 2 + 2^3 + 2^7 + 2^8 + 2^15 + 2^18 + 2^19 + 2^20 + 2^21 + 2^26 + O(2^28) : 2^-3 + 2^-2 + 2^-1 + 2 + 2^2 + 2^5 + 2^6 + 2^7 + 2^9 + 2^11 + 2^14 + 2^15 + 2^19 + 2^20 + 2^23 + 2^24 + 2^25 + O(2^26) : 1 + O(2^60))

comment:9 Changed 2 years ago by edgarcosta

  • Status changed from needs_review to positive_review

Ignore my comment, I will just move that to a new ticket

comment:10 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name is missing...

comment:11 Changed 2 years ago by edgarcosta

  • Reviewers set to Edgar Costa
  • Status changed from needs_work to positive_review

comment:12 Changed 2 years ago by vbraun

  • Branch changed from u/wuthrich/tate_curve_prec_28645 to 9c537fbbeb0f6106cc637d5aa6e1cfff9437dd4f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.