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

Fix .multiplication_by_m when m is not coprime to characteristic (#6413) #37096

Merged
merged 12 commits into from
Feb 13, 2024

Conversation

grhkm21
Copy link
Contributor

@grhkm21 grhkm21 commented Jan 18, 2024

Almost fixes #6413 - doesn't work for function fields

Compute pth multiplication coordinate maps by using isogenies.

The multivariate substitution step is VERY slow, and I don't know how to fix it. I think the reason might also be the elements live in the fraction field.

Thanks @yyyyx4 for the idea! #sd123 🚀

Compute pth multiplication coordinate maps by using isogenies.

Thanks @yyyyx4 for the idea!
@tscrim tscrim requested a review from yyyyx4 January 22, 2024 08:16
@tscrim
Copy link
Collaborator

tscrim commented Jan 22, 2024

@yyyyx4 Can you be reviewing this (and other such related PRs from sd123)?

@yyyyx4
Copy link
Member

yyyyx4 commented Jan 22, 2024

@yyyyx4 Can you be reviewing this (and other such related PRs from sd123)?

I wasn't sure if I'm "allowed" to review many of these since I suggested quite a few of them as possible #sd123 projects myself (and in some cases even contributed to the patch with ideas and advice). Is there a policy about this?

@tscrim
Copy link
Collaborator

tscrim commented Jan 22, 2024

@yyyyx4 Can you be reviewing this (and other such related PRs from sd123)?

I wasn't sure if I'm "allowed" to review many of these since I suggested quite a few of them as possible #sd123 projects myself (and in some cases even contributed to the patch with ideas and advice). Is there a policy about this?

You are definitely allowed to review this. It can even be 2 authors who are the same reviewers but they cross-check each other. Although with the new GH system, this has become more troublesome to do in practice. I can still do a final double-check if you prefer too.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jan 24, 2024

Thanks for the review and sorry for the mess, I didn't really think through the code and just pushed what worked for the first example :P

src/sage/schemes/elliptic_curves/ell_finite_field.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_generic.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_generic.py Outdated Show resolved Hide resolved
my = ((2*y+a1*x+a3)*mx.derivative(x)/m - a1*mx-a3)/2

return mx, my

def multiplication_by_m_isogeny(self, m):
Copy link
Member

Choose a reason for hiding this comment

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

I acknowledge that we are technically "allowed" to remove this method now, but I'm wondering if it's a good idea at this point: Debian and Ubuntu are still on Sage 9.5, so most of their users have never even seen the deprecation warning. Since keeping this method in Sage a little longer is essentially free of harm, I'd suggest to do so.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jan 30, 2024

I've left the deprecated multiplication_by_m_isogeny. Hopefully the package installers update their versions soon, ha

Copy link

Documentation preview for this PR (built with commit 929d0e6; changes) is ready! 🎉

Copy link
Member

@yyyyx4 yyyyx4 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@vbraun vbraun merged commit 138b265 into sagemath:develop Feb 13, 2024
22 checks passed
Comment on lines +2449 to +2453
sage: p = 7
sage: K.<a> = GF(p^2)
sage: E = EllipticCurve(K, [K.random_element(), K.random_element()])
sage: E.multiplication_by_m(p * 2)[0] == E.multiplication_by_m(p * 2, x_only=True)
True
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails at random with probability ~ 1/49 or something like that (i.e. when the discriminant is 0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix this in a separate PR since this is merged already. Sorry for not spotting this earlier (there's one similar above)

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2024
…ctest

    
The long-standing issue sagemath#6413 was resolved in sagemath#37096, but there is still
one `# known bug` marker for it in
`src/sage/schemes/elliptic_curves/hom_scalar.py`. Let's remove it.

(Incidentally, the claimed output for the invocation that used to not
work was slightly wrong, too.)
    
URL: sagemath#37328
Reported by: Lorenz Panny
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 19, 2024
…ctest

    
The long-standing issue sagemath#6413 was resolved in sagemath#37096, but there is still
one `# known bug` marker for it in
`src/sage/schemes/elliptic_curves/hom_scalar.py`. Let's remove it.

(Incidentally, the claimed output for the invocation that used to not
work was slightly wrong, too.)
    
URL: sagemath#37328
Reported by: Lorenz Panny
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 22, 2024
…ctest

    
The long-standing issue sagemath#6413 was resolved in sagemath#37096, but there is still
one `# known bug` marker for it in
`src/sage/schemes/elliptic_curves/hom_scalar.py`. Let's remove it.

(Incidentally, the claimed output for the invocation that used to not
work was slightly wrong, too.)
    
URL: sagemath#37328
Reported by: Lorenz Panny
Reviewer(s):
@grhkm21 grhkm21 deleted the grhkm/issue-6413 branch February 25, 2024 17:22
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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.

Retrieving the multiplication-by-p isogeny on elliptic curves over fields of characteristic p fails.
6 participants