Skip to content

Conversation

@drago-96
Copy link
Contributor

@drago-96 drago-96 commented Jan 18, 2024

There is a bug in the new classical_modular_polynomial function created in #36190.

A simple example to reproduce the bug is the following

p = 2^216 * 3^137 - 1
F.<i> = GF(p^2, modulus=[1,0,1])
E = EllipticCurve(F, [0, 6, 0, 1, 0])
classical_modular_polynomial(2, E.j_invariant())

This will fail with a TypeError.

The bug is due to the interface with the pari function polmodular. In particular, contrary to the documentation, the polmodular function will only evaluate $\Phi_\ell(j, Y)$ for $j$-invariants that are defined over $\mathbb F_p$, and not over any generic finite field.
If however j.parent() is a generic finite field, but j itself is defined over the prime field, pari will evaluate that and the current implementation fails to convert back to a sage polynomial.

The proposed fix is to cast the result of the pari call to ZZ['Y'], since the result of polmodular currently returns a polynomial in the correct $\mathbb Z/n\mathbb Z[Y]$.

#sd123

True
sage: p = 2^216 * 3^137 - 1
sage: F.<i> = GF(p^2, modulus=[1,0,1])
sage: l = random_prime(50)
Copy link
Contributor

Choose a reason for hiding this comment

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

you could use the syntax GF((p,2),...) to avoid factoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know this constructor!

@github-actions
Copy link

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

@pjbruin
Copy link
Contributor

pjbruin commented Jan 19, 2024

Shouldn't the result be converted into a polynomial over Z/pZ where p is the characteristic of the finite field?

@grhkm21
Copy link
Contributor

grhkm21 commented Jan 20, 2024

Shouldn't the result be converted into a polynomial over Z/pZ where p is the characteristic of the finite field?

Why? I think the PR is fine, it returns a polynomial over parent(j), which is what you want normally anyways (to compute its roots later etc.).

@pjbruin
Copy link
Contributor

pjbruin commented Jan 20, 2024

You are right. I was puzzled by the conversion to Z[Y], and did not notice that changing Z to Z/pZ would not make a difference, due to the final conversion to R.

The fact that polmodular returns a polynomial over a different field than the one in which j lies does seem to be a possible PARI bug and should probably also be reported there.

Copy link
Contributor

@pjbruin pjbruin left a comment

Choose a reason for hiding this comment

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

Likely a PARI bug, the patch looks like a good workaround to me

@vbraun vbraun merged commit 6224890 into sagemath:develop Jan 22, 2024
@pjbruin
Copy link
Contributor

pjbruin commented Jan 22, 2024

@drago-96
Copy link
Contributor Author

Thanks for the reporting and fixing in PARI!

Eloitor pushed a commit to Eloitor/sagemath that referenced this pull request Jan 29, 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.

7 participants