Skip to content
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

add missing EllipticCurveHom methods to Îlu isogenies #34614

Closed
yyyyx4 opened this issue Sep 29, 2022 · 20 comments
Closed

add missing EllipticCurveHom methods to Îlu isogenies #34614

yyyyx4 opened this issue Sep 29, 2022 · 20 comments

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Sep 29, 2022

The EllipticCurveHom_velusqrt class introduced in #34303 inherits from EllipticCurveHom (#32388, #32502), but it doesn't yet implement all of the required methods. In this patch, we add them.

Diff without the dependency: sagemath/sagetrac-mirror@7326051...8cb0f29

Depends on #34410

CC: @defeo @JohnCremona @tscrim @kwankyu

Component: elliptic curves

Author: Lorenz Panny

Branch/Commit: 8cb0f29

Reviewer: Travis Scrimshaw, Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/34614

@yyyyx4 yyyyx4 added this to the sage-9.8 milestone Sep 29, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2022

Changed commit from 0a38cc5 to 726f2db

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7a64e8cMerge branch 'public/change_default_composition_for_elliptic_curve_isogenies'
726f2dbadd more EllipticCurveHom methods to EllipticCurveHom_velusqrt

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7326051Merge tag '9.8.beta1' into public/change_default_composition_for_elliptic_curve_isogenies
c6bbbc3add more EllipticCurveHom methods to EllipticCurveHom_velusqrt

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2022

Changed commit from 726f2db to c6bbbc3

@yyyyx4

This comment has been minimized.

@yyyyx4

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Oct 1, 2022

comment:6

Code wise, basically looks good (although I would like the long doctest output lines broken up a little bit). Could someone check the math?

@tscrim
Copy link
Collaborator

tscrim commented Oct 1, 2022

Reviewer: Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 7, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

8e95731Minor edits mostly on spaces
df1024dMove as_morphism()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 7, 2022

Changed commit from c6bbbc3 to df1024d

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 7, 2022

comment:8

I made minor edits.

I took this opportunity to move the method as_morphism() to the class EllipticCurveHom so that it works with Îlu isogenies. The method may be useful to check elliptic curve morphisms, though it is reasonably fast only for small examples.

One remark: There are notes like

    def rational_maps(self):
        r"""
        Return the pair of explicit rational maps of this Îlu isogeny
        as fractions of bivariate polynomials in `x` and `y`.

        .. NOTE::

            The data returned by this method has size linear in the degree.

        EXAMPLES::  

The meaning of the note is obscure to me. How is the size of the data measured? If the data is a polynomial, then it may mean "the degree of the returned polynomial is linear in the degree of the isogeny". In this case, the data is two rational functions. You need to be explicit about what is the "size" of rational functions. I suggest that you rewrite the notes or remove them (as they are, the notes are not so useful).

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 7, 2022

comment:9

Replying to Kwankyu Lee:

How is the size of the data measured?

It's literally the size of the object, as in "bytes of memory it takes to store".

The purpose of these remarks is to point out that merely writing down the result of calling these methods already requires time Ω(ℓ), which means the √élu speedup (complexity Õ(√ℓ)) cannot apply to them. Perhaps that point should be made clearer.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 7, 2022

comment:10

Replying to Lorenz Panny:

Replying to Kwankyu Lee:

How is the size of the data measured?

It's literally the size of the object, as in "bytes of memory it takes to store".

Okay.

The purpose of these remarks is to point out that merely writing down the result of calling these methods already requires time Ω(ℓ), which means the √élu speedup (complexity Õ(√ℓ)) cannot apply to them. Perhaps that point should be made clearer.

I now see what you mean by the note. Then if you want to keep those notes, I suggest that you write this explanation (implication) for these notes, once and for all, at the head of the file hom_velusqrt.py. Non-experts need the explanation.

Otherwise I am positive to the branch.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 7, 2022

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Kwankyu Lee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 7, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

8cb0f29explain "output has linear size" remarks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 7, 2022

Changed commit from df1024d to 8cb0f29

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 7, 2022

comment:13

Thanks! I added a note about it as you suggested.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 7, 2022

comment:14

Thank you!

@yyyyx4

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Oct 16, 2022

kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
… point orders

This is a follow-up to sagemath#32786 with the following changes:

- Copy curve orders between domain and codomain for all
`EllipticCurveHom` instances of nonzero degree, rather than (previously)
just `EllipticCurveIsogeny` objects.
- Copy point orders when pushing a point through an isomorphism.
- Copy point orders when pushing a point through an isogeny of degree
coprime to the point order.
- Rearrange some computations in the Îlu code (sagemath#34303, sagemath#34614) to make
(better) use of cached orders; in particular, this unbreaks the use of
`.set_order()` on the kernel point prior to passing it to
`EllipticCurveHom_velusqrt`. [Thanks to Jonathan Komada Eriksen for
reporting this last issue.]

URL: https://trac.sagemath.org/34732
Reported by: lorenz
Ticket author(s): Lorenz Panny
Reviewer(s): Travis Scrimshaw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants