Improved algorithm choice for isogeny computation #37242
Merged
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
GiacomoPope
reviewed
Feb 5, 2024
|
Thank you! I applied all your suggestions (my |
GiacomoPope
added
s: positive review
s: needs work
and removed
s: needs work
s: positive review
labels
|
I was about to mark this as completed, but somehow |
|
Sorry, I keep messing up with git. I deleted it with a new commit, is that fine? Or should I do something different? |
GiacomoPope
approved these changes
Feb 6, 2024
|
Thanks! |
|
Test failure, see the CI at https://github.com/sagemath/sage/actions/runs/7805562652 |
|
Fixed, I also noticed another failure in the coding style so I fixed that one as well. |
grhkm21
reviewed
Feb 10, 2024
grhkm21
reviewed
Feb 10, 2024
|
If CI passes then it's all good. This already has |
|
Documentation preview for this PR (built with commit b4408ab; changes) is ready! 🎉 |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Feb 18, 2024
Improved the logic for algorithm choice in
`EllipticCurve_field.isogeny()`:
- if we have multiple points, `factored` is used;
- if the order is known and composite, `factored` is used;
- if the order is known and (pseudo-)prime, we chose between the
traditional Velù formulae and Velù-sqrt, depending on a parameter; this
parameter may be supplied by the user with the `velu_sqrt_bound`
argument, and if `None` a global parameter is used (see below);
- in all the other cases we fall back to the traditional isogeny
algorithm.
The new `velu_sqrt_bound`:
- can be supplied as a parameter when calling `.isogeny()`;
- if supplied, is recursively passed to all calls to `factored`, such
that each single isogeny computation has the same bound;
- if not supplied, a global value is used instead; this is stored in the
object `_velu_sqrt_bound` from
`sage.schemes.elliptic_curves.hom_velusqrt`, and is initially set to
1000 (due to empirical observations) but can be manually set at runtime
by the user
#sd123
URL: sagemath#37242
Reported by: Riccardo Invernizzi
Reviewer(s): Giacomo Pope, grhkm21, Riccardo Invernizzi
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Feb 19, 2024
Improved the logic for algorithm choice in
`EllipticCurve_field.isogeny()`:
- if we have multiple points, `factored` is used;
- if the order is known and composite, `factored` is used;
- if the order is known and (pseudo-)prime, we chose between the
traditional Velù formulae and Velù-sqrt, depending on a parameter; this
parameter may be supplied by the user with the `velu_sqrt_bound`
argument, and if `None` a global parameter is used (see below);
- in all the other cases we fall back to the traditional isogeny
algorithm.
The new `velu_sqrt_bound`:
- can be supplied as a parameter when calling `.isogeny()`;
- if supplied, is recursively passed to all calls to `factored`, such
that each single isogeny computation has the same bound;
- if not supplied, a global value is used instead; this is stored in the
object `_velu_sqrt_bound` from
`sage.schemes.elliptic_curves.hom_velusqrt`, and is initially set to
1000 (due to empirical observations) but can be manually set at runtime
by the user
#sd123
URL: sagemath#37242
Reported by: Riccardo Invernizzi
Reviewer(s): Giacomo Pope, grhkm21, Riccardo Invernizzi
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.
Improved the logic for algorithm choice in
EllipticCurve_field.isogeny():factoredis used;factoredis used;velu_sqrt_boundargument, and ifNonea global parameter is used (see below);The new
velu_sqrt_bound:.isogeny();factored, such that each single isogeny computation has the same bound;_velu_sqrt_boundfromsage.schemes.elliptic_curves.hom_velusqrt, and is initially set to 1000 (due to empirical observations) but can be manually set at runtime by the user#sd123