-
-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sums of elliptic-curve morphisms #36637
sums of elliptic-curve morphisms #36637
Conversation
b717065
to
5de7eb6
Compare
I can review this, but not until Tuesday, sorry. Feel free to proceed without me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made a few comments, at least one of which requires a change, and will think further about the question of computing deg(phi+psi) more directly.
B += phi.degree() + 2*sqrt(B * phi.degree()) | ||
return integer_floor(B) | ||
|
||
def _compute_degree(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel sure that there must be a way to compute the degree of the sum of two morphisms; if so you caould call it repeateldy here. But I'll need to think about that a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose what definitely works is the identity
My stupid mistake, apologies.
…On Tue, 7 Nov 2023, 18:56 Lorenz Panny, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/sage/schemes/elliptic_curves/ell_point.py
<#36637 (comment)>:
> @@ -1359,9 +1359,10 @@ def set_order(self, value=None, *, multiple=None, check=True):
raise ValueError('Value %s illegal for point order' % value)
E = self.curve()
q = E.base_ring().cardinality()
- low, hi = Hasse_bounds(q)
- if value > hi:
- raise ValueError('Value %s illegal: outside max Hasse bound' % value)
+ if q < oo:
+ _, hi = Hasse_bounds(q)
+ if value > hi:
Really? It looks correct to me; for example, Hasse_bounds(101) returns
$(82,122)$.
—
Reply to this email directly, view it on GitHub
<#36637 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXU32LPDLC7KEDDUE7266TYDJ76BAVCNFSM6AAAAAA64LT3T6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJYGQ4TCNJQGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes, that's what I was thinking of. Without experimenting I cannot tell
which would be fastest.
…On Tue, 7 Nov 2023, 18:55 Lorenz Panny, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/sage/schemes/elliptic_curves/hom_sum.py
<#36637 (comment)>:
> + ::
+
+ sage: E = EllipticCurve(GF(443), [1,1])
+ sage: pi = E.frobenius_endomorphism()
+ sage: m1 = E.scalar_multiplication(1)
+ sage: (pi - m1)._degree_bound()
+ 486
+ sage: (pi - m1).degree()
+ 433
+ """
+ B = ZZ(0)
+ for phi in self._phis:
+ B += phi.degree() + 2*sqrt(B * phi.degree())
+ return integer_floor(B)
+
+ def _compute_degree(self):
I suppose what definitely works is the identity $\deg(\varphi+\psi) =
\deg(\varphi)+\deg(\psi)+\mathrm{tr}(\varphi\widehat\psi)$, now that we
have #35546 <#35546>. But there
should be cases where computing multiple such trace pairings will certainly
be slower than the current one-shot method, so this might require some
thinking...
—
Reply to this email directly, view it on GitHub
<#36637 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXU32JE3LKJP4YTVV4UOIDYDJ72RAVCNFSM6AAAAAA64LT3T6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJYGQ4TAMJVHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
OK!
…On Tue, 7 Nov 2023, 19:00 Lorenz Panny, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/sage/schemes/elliptic_curves/hom_sum.py
<#36637 (comment)>:
> + M = self._degree_bound() + 1
+ deg = Mod(0,1)
+ for l in Primes():
+ if deg.modulus() >= M:
+ break
+ try:
+ P = point_of_order(self._domain, l)
+ except ValueError:
+ continue # supersingular and l == p
+
+ Q = self.dual()._eval(self._eval(P))
+ d = discrete_log(Q, P, ord=l, operation='+')
+ deg = deg.crt(Mod(d, l))
+
+ self._degree = ZZ(deg)
+ self.dual()._degree = self._degree
In this class .dual() is marked @cached_method, so the same object will
be returned on subsequent calls.
—
Reply to this email directly, view it on GitHub
<#36637 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXU32KH7PYM3VZODILVCMDYDKALNAVCNFSM6AAAAAA64LT3T6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJYGQ4TQOBSGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
SageMath version 10.2.rc0, Release Date: 2023-11-05
Documentation preview for this PR (built with commit fbe441e; changes) is ready! 🎉 |
All the suggestions I made in my review and follow-ups have been implemented (or corrected), so this can have a positive review from me assuming that tests pass. |
Thanks a lot, @JohnCremona! The failures appear to be unrelated to these changes. |
Here we add a new `EllipticCurveHom` child class representing formal sums of elliptic-curve morphisms with the same domain and codomain. One of the main features is a conversion function from formal sums to a more explicit representation of the same morphism as an isogeny chain, which will come in handy for endomorphism-ring-related computations. In the process, we also generalize `point_of_order()` from primes to prime powers. There is certainly a lot of potential for optimizations; some ideas are mentioned in the code. Since I think it's fair to say that this version is already much better than what we currently have for sums of isogenies (i.e., nothing), I'd suggest merging this as is and dealing with bottlenecks later as they arise. Cc: @JohnCremona @defeo @GiacomoPope @remyoudompheng URL: sagemath#36637 Reported by: Lorenz Panny Reviewer(s): John Cremona, Lorenz Panny
Here we add a new
EllipticCurveHom
child class representing formal sums of elliptic-curve morphisms with the same domain and codomain. One of the main features is a conversion function from formal sums to a more explicit representation of the same morphism as an isogeny chain, which will come in handy for endomorphism-ring-related computations.In the process, we also generalize
point_of_order()
from primes to prime powers.There is certainly a lot of potential for optimizations; some ideas are mentioned in the code. Since I think it's fair to say that this version is already much better than what we currently have for sums of isogenies (i.e., nothing), I'd suggest merging this as is and dealing with bottlenecks later as they arise.
Cc: @JohnCremona @defeo @GiacomoPope @remyoudompheng