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: |
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
comment:2 Changed 8 years ago by
- Branch set to u/chapoton/15964
- Commit set to d3224bbd5f0e51f3230da8e84c8f73196e634eb4
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
Hello, this seems to be an easy ticket, and should be rather simple to review.
comment:4 Changed 8 years ago by
- Reviewers set to Peter Bruin
- Status changed from needs_review to positive_review
comment:5 Changed 8 years ago by
- Branch changed from u/chapoton/15964 to d3224bbd5f0e51f3230da8e84c8f73196e634eb4
- Resolution set to fixed
- Status changed from positive_review to closed
Alternative: changing
ZZ(value)
tovalue.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.