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

direct computation of .formal()[1] for elliptic-curve morphisms #33865

Closed
yyyyx4 opened this issue May 18, 2022 · 10 comments
Closed

direct computation of .formal()[1] for elliptic-curve morphisms #33865

yyyyx4 opened this issue May 18, 2022 · 10 comments

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented May 18, 2022

Follow-up to #33216: We can compute .formal()[1] directly, i.e., without going through .formal(). This is beneficial as the latter uses the rational maps, which can be very expensive.

  • Composite isogenies have multiplicative .formal()[1].
  • Isogenies using Vélu's or Kohel's formulas are normalized, hence .formal()[1] == 1. We only need to account for pre- and post-isomorphism.
  • Weierstrass isomorphisms (u,r,s,t) have .formal()[1] == u.

Same example as for #33216:

E = EllipticCurve(j=GF(431^2)(4))
phi = E.isogeny(2^99*E.gens()[0])
_ = phi.dual()
%timeit phi._EllipticCurveIsogeny__dual = None; phi.dual()

Sage 9.6:

537 ms ± 6.75 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

This branch:

294 ms ± 1.71 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

CC: @defeo @JohnCremona @categorie

Component: elliptic curves

Author: Lorenz Panny

Branch/Commit: fb571d3

Reviewer: John Cremona

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

@yyyyx4 yyyyx4 added this to the sage-9.7 milestone May 18, 2022
@JohnCremona
Copy link
Member

comment:2

This looks great, and well worth doing. My only comment is that all these _scaling_factor() methods should be user-acessible, i.e. not have the underscore, since this is a useful number associated to an isogeny -- one that I have often needed to access.

At the same time, perhaps we can have in the docstring a definition of this "scaling factor". I would say something like this: it is the factor u (in the base field) such that $\phi^*\omega_2 = u \omega_1$, where $\phi:E_1\to E_2$ is the isogeny and for $i=1,2$, $\omega_i$ is the standard Weierstrass differential, i.e. $dx/(2y+a_1x+a_3)$.

[Sorry, I forgot that trac does not understand latex.]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2022

Changed commit from 2e88ff0 to fb571d3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2022

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

fb571d3turn .scaling_factor() into a public function

@yyyyx4
Copy link
Member Author

yyyyx4 commented May 18, 2022

comment:4

Thank you for the suggestion! I changed it to .scaling_factor() and added your definition (with minor tweaks) to the documentation.

@JohnCremona
Copy link
Member

comment:5

Sorry about the delay in coming back to this.
I would give a positive review, but do not understand the "test fail" flag. I looked at the logs and tere were no failing tests, the log just stops in the middle.

Is it possible to get the test to re-run?

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jun 6, 2022

comment:6

The patchbot log here on Trac is fully green and looks complete over here. Maybe your browser hadn't finished loading it all the way to the end?

On GitHub Actions (the source of the alarmingly red "Build & Test | failing" icon), there is a single test failure, but it seems entirely unrelated to this patch:

sage -t --random-seed=255890665791265155015760495860109238381 sage/parallel/map_reduce.py
**********************************************************************
File "sage/parallel/map_reduce.py", line 1151, in sage.parallel.map_reduce.RESetMapReduce.start_workers
Failed example:
    all(not w.is_alive() for w in S._workers)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of  10 in sage.parallel.map_reduce.RESetMapReduce.start_workers
    [294 tests, 1 failure, 41.75 s]

@JohnCremona
Copy link
Member

comment:7

Thanks for the quick help. I do now see the whole log file. I'll give this a positive review then.

@JohnCremona
Copy link
Member

Reviewer: John Cremona

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jun 6, 2022

comment:8

Thank you!

@vbraun
Copy link
Member

vbraun commented Jun 12, 2022

Changed branch from public/elliptic_curve_morphisms_scaling_factor to fb571d3

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

3 participants