Opened 8 years ago

Closed 8 years ago

#15964 closed defect (fixed)

Remove superfluous import statement in ell_point

Reported by: cremona Owned by:
Priority: minor Milestone: sage-6.2
Component: elliptic curves Keywords:
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: d3224bb (Commits, GitHub, GitLab) Commit: d3224bbd5f0e51f3230da8e84c8f73196e634eb4
Dependencies: Stopgaps:

Status badges

Description

As reported on sage-devel, in Sage 6.1.1 this works as designed:

sage: N = 1715761513
sage: E = EllipticCurve(Integers(N),[3,-13])
sage: P = E(2,1)
sage: LCM([2..60])*P
...ZeroDivisionError: Inverse of 1520944668 does not exist (characteristic = 1715761513 = 26927*63719)

but this fails in the wrong way:

N = 35
E = EllipticCurve(Integers(N),[5,1])
P = E(0,1)
LCM([2..6])*P
...
UnboundLocalError: local variable 'ZZ' referenced before assignment

The problem is that there are two places where the fail condiction can be triggered (and in this situation this happens by design, as it is used to detect the fact that the modulus is not prime and factor it): one in addition of points and one in duplication. In the latter there is an explicit "import ZZ" while in the former there is not. This could be fixed by deleting the import on line 708 (thus relying on the import at the top of the file in line 130), or by deleting line 130 (since ZZ is not referred to elsehere in the file) and inderting a copy of line 708 before line 696.

Change History (5)

comment:1 Changed 8 years ago by nbruin

Alternative: changing ZZ(value) to value.lift(), so that ZZ doesn't need to be imported at all.

Really we'd need to check that R is a quotient of ZZ before attempting any of this, not just if R is finite.

In general: doing in import of ZZ at module instantiation time should be cheap enough, but it's not required at all here, since integers mod N come with a method to get a representing integer.

comment:2 Changed 8 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/15964
  • Commit set to d3224bbd5f0e51f3230da8e84c8f73196e634eb4
  • Status changed from new to needs_review

Here is a branch, please review


New commits:

d3224bbtrac #15964 using Integer instead of ZZ

comment:3 Changed 8 years ago by chapoton

Hello, this seems to be an easy ticket, and should be rather simple to review.

comment:4 Changed 8 years ago by pbruin

  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_review

comment:5 Changed 8 years ago by vbraun

  • Branch changed from u/chapoton/15964 to d3224bbd5f0e51f3230da8e84c8f73196e634eb4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.