generalize EllipticCurve_field.division_field() to composite orders #35936
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yyyyx4
force-pushed
the
public/generalize_elliptic_curve_division_field_to_composite_orders
branch
2 times, most recently
from
7266f83
to
bb9dc2a
Compare
|
I am looking at this now... |
JohnCremona
approved these changes
Jul 24, 2023
|
Merge conflict |
yyyyx4
force-pushed
the
public/generalize_elliptic_curve_division_field_to_composite_orders
branch
from
bb9dc2a
to
1371efb
Compare
|
Trivial rebase. |
|
As far as I can see the Build & test failures are nothing to do with this PR. |
|
On 32-bit: |
…ield_to_composite_orders SageMath version 10.2.rc0, Release Date: 2023-11-05
|
Does the automatic testing inclde 32-bit testing? I hope so. I'll be happy with this once tests pass (and I know that I said that before the necessary fix in 560bf2a). |
|
Documentation preview for this PR (built with commit dfd4aa5; changes) is ready! 🎉 |
|
I don't think it does, but from experience it looks like @vbraun will run tests on a 32-bit machine manually. Anyway, if the result consistently matches the one in #35936 (comment) then the updated doctest should definitely pass... |
|
@vbraun Can you explain the "needs work" tag? |
|
I interpreted the "needs work" label to be for the failing 32-bit doctest, which should be fixed now. |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 4, 2023
…composite orders
The `.division_field()` method is currently restricted to prime orders
for no serious reason. This patch makes it work for composite orders,
generalizing [an observation of @JohnCremona](https://github.com/sagemat
h/sage/issues/11905#issuecomment-1417417647) in the process.
Note: I'm not sure if this is the optimal approach. I also played around
with building the extension as a tower corresponding to the
factorization of $n$, but (at least in Sage) it seemed significantly
slower than the version here.
Fixes sagemath#24340.
URL: sagemath#35936
Reported by: Lorenz Panny
Reviewer(s): John Cremona
yyyyx4
deleted the
public/generalize_elliptic_curve_division_field_to_composite_orders
branch
|
On recent betas (at least 10.3.beta5 and beta6) but not on 10.2: FWIW, 528238639 is prime. |
|
That looks like #36832. |
Thanks, I searched |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
.division_field()method is currently restricted to prime orders for no serious reason. This patch makes it work for composite orders, generalizing an observation of @JohnCremona in the process.Note: I'm not sure if this is the optimal approach. I also played around with building the extension as a tower corresponding to the factorization of$n$ , but (at least in Sage) it seemed significantly slower than the version here.
Fixes #24340.