Skip to content

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Oct 1, 2025

Computing a product of a collection of linear polynomials is best done using a balanced product tree.

Example:

sage: E = EllipticCurve(GF(16411), [1,0])
sage: P = E.lift_x(25)
sage: phi = E.isogeny(P)
sage: assert phi._EllipticCurveIsogeny__algorithm == 'velu'
sage: %time _ = phi.kernel_polynomial()

Current develop:

CPU times: user 25.6 ms, sys: 0 ns, total: 25.6 ms
Wall time: 25.7 ms

This branch:

CPU times: user 13.7 ms, sys: 0 ns, total: 13.7 ms
Wall time: 13.8 ms

Copy link
Contributor

@GiacomoPope GiacomoPope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change, it does make me wonder if it would be good to make a polynomial from roots method. I know flint has methods for this and maybe NTL does too?

for xQ in self.__kernel_mod_sign.keys():
psi *= x - invX(xQ)
from sage.misc.misc_c import prod
psi = prod([x - invX(xQ) for xQ in self.__kernel_mod_sign.keys()]) # building the list is not redundant; this is slightly faster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really? prod delegates to iterator_prod if the input is a generator. Where does the overhead come from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the overhead might be because Python can inline the list comprehension, but not the generator expression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesnt prod use a binary tree for the product when given a list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last time I checked, the difference was that passing an iterator makes it use a different (suboptimal) tree structure since the total length is not a priori known.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it again, I'm not sure if that is the reason: iterator_prod() does promise that the tree will be balanced... So you might be right, @user202729.

Copy link

github-actions bot commented Oct 1, 2025

Documentation preview for this PR (built with commit c9545c8; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 1, 2025

Good change, it does make me wonder if it would be good to make a polynomial from roots method. I know flint has methods for this and maybe NTL does too?

Nice suggestion. Should we add the .from_roots() abstraction here already or do you want to move it to a separate patch?

@GiacomoPope
Copy link
Contributor

I had a look (albeit quickly) for the function in NTL and couldn't see it. So maybe it's just flint that supports it? I dunno what's best. Maybe just a generic python implementation and then a call to library specific functions if they exist.

This feels like a good place to introduce the patch but also it could be in a new PR to avoid this taking up more of your time.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
sagemathgh-40948: speed up construction of kernel polynomial for Vélu isogeny using product tree
    
Computing a product of a collection of linear polynomials is best done
using a balanced product tree.

Example:
```sage
sage: E = EllipticCurve(GF(16411), [1,0])
sage: P = E.lift_x(25)
sage: phi = E.isogeny(P)
sage: assert phi._EllipticCurveIsogeny__algorithm == 'velu'
sage: %time _ = phi.kernel_polynomial()
```

Current `develop`:
```
CPU times: user 25.6 ms, sys: 0 ns, total: 25.6 ms
Wall time: 25.7 ms
```

This branch:
```
CPU times: user 13.7 ms, sys: 0 ns, total: 13.7 ms
Wall time: 13.8 ms
```
    
URL: sagemath#40948
Reported by: Lorenz Panny
Reviewer(s): Giacomo Pope, Lorenz Panny, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
sagemathgh-40948: speed up construction of kernel polynomial for Vélu isogeny using product tree
    
Computing a product of a collection of linear polynomials is best done
using a balanced product tree.

Example:
```sage
sage: E = EllipticCurve(GF(16411), [1,0])
sage: P = E.lift_x(25)
sage: phi = E.isogeny(P)
sage: assert phi._EllipticCurveIsogeny__algorithm == 'velu'
sage: %time _ = phi.kernel_polynomial()
```

Current `develop`:
```
CPU times: user 25.6 ms, sys: 0 ns, total: 25.6 ms
Wall time: 25.7 ms
```

This branch:
```
CPU times: user 13.7 ms, sys: 0 ns, total: 13.7 ms
Wall time: 13.8 ms
```
    
URL: sagemath#40948
Reported by: Lorenz Panny
Reviewer(s): Giacomo Pope, Lorenz Panny, user202729
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants