Implement EllipticCurve_with_order
#37119
Conversation
|
The function can be sped up on some small edge cases when #37110 is finished, but it works as is. |
Someone that's experienced with CM method should check whether my implementation truly returns all curves (over a prime finite field) with the given order. In particular, I might be missing $p = 2$?
|
I believe this is ready for review :) |
| @@ -1596,17 +1596,20 @@ def small_prime_value(self, Bmax=1000): | |||
| raise ValueError("Unable to find a prime value of %s" % self) | |||
| B += 10 | |||
|
|
|||
| def solve_integer(self, n, *, algorithm="general"): | |||
| def solve_integer(self, n, *, algorithm="general", flag=2): | |||
There was a problem hiding this comment.
In my opinion we shouldn't expose PARI's non-human-readable APIs: Can flag be passed by a sensible choice of keyword arguments instead?
|
Is this being held up because of the pari flag? |
|
No, I deferred it to another issue already #37364 |
|
I'm a coauthor, so I dont think I should review it further, but if @yyyyx4's only comment left is about the PARI flag and this is being delayed then maybe a positive review is OK? |
| @@ -1741,7 +1753,7 @@ def twists(self): | |||
|
|
|||
| sage: # needs sage.rings.finite_rings | |||
| sage: K = GF(2**7) | |||
| sage: [E.ainvs() for E in EllipticCurve(j=K(1)).twists()] | |||
| sage: [E.ainvs() for E in EllipticCurve(j=K(1)).twists()] # random | |||
There was a problem hiding this comment.
I agree with Lorenz that this could be sorted and random could be removed, or is the issue that the twists themselves are random. Maybe im being stupid here.
|
Sorry for the snail pace work. I've reverted the changes related to precomputing order, since they don't affect correctness, only speed. They're at #37110 anyways. Next, I'll work on your reviews |
|
|
|
Documentation preview for this PR (built with commit a1eb6c2; changes) is ready! 🎉 |
|
Thanks! |
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Me and @grhkm21 suggest this diff against develop that implements the `EllipticCurve_with_prime_order(N)` constructor. Using the prime order `N` in input, this method finds another prime `p` and constructs an elliptic curve `E/Fp` with `#E(Fp) = N`. It follows Algorithm 2.2 of the paper [Constructing Elliptic Curves of prime order](https://arxiv.org/abs/0712.2022) by Bröker and Stevenhagen. The running time is quite random depending on the input parameter but can turn out to be fast for some larger values (≃ 256 bits primes). It's also worth noticing that some values will make this function run for a **very** long time. There had been a [PR](sagemath#37119) by @grhkm21 and @GiacomoPope that implements the `EllipticCurve_with_order()` method. This PR would intuitively fit nice into their work but I felt uncomfortable with it returning an iterator. I felt like returning a single curve was more handy so I implemented this method in a separate function that does so but I'm open to suggestions if this is of any interest to the community. Fixes sagemath#38342 ### 📝 Checklist - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38341 Reported by: grnx Reviewer(s): grhkm21, grnx, Vincent Macri
This function returns a curve / an iterator of curves with a given order. These are generated using the CM method.
Also fix #37131 (13 year old bug).
Co-authored-by: @GiacomoPope #sd123 🚀