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

Incorrect assumptions about polynomial in is_HCP() #37471

Closed
2 tasks done
GiacomoPope opened this issue Feb 25, 2024 · 0 comments · Fixed by #37443
Closed
2 tasks done

Incorrect assumptions about polynomial in is_HCP() #37471

GiacomoPope opened this issue Feb 25, 2024 · 0 comments · Fixed by #37443

Comments

@GiacomoPope
Copy link
Contributor

GiacomoPope commented Feb 25, 2024

Steps To Reproduce

The current code has the following snippet:

        p = next_prime(p)
        n += 1
        fp = f.change_ring(GF(p))
        # Compute X^p-X mod fp
        z = fp.parent().gen()
        r = pow(z, p, fp) - z
        d = r.gcd(fp).degree()  # number of roots mod p
        if d == 0:
            continue
        if not fp.is_squarefree():
            continue
        if d < h and d not in h2list:
            return zero
        jp = fp.any_root(degree=1, assume_squarefree=True, assume_distinct_deg=True)

The problem is, fp.any_root() is called with assume_distinct_deg True. In this case, the polynomial is expected to be the product of irreducible polynomials of degree degree (in this case linear polynomials).

However, this is a typo. The polynomial is not of this form. The correct polynomial to use is the polynomial

r = pow(z, p, fp) - z
r = r.gcd(fp)

As a result of this, tests such as:

from sage.schemes.elliptic_curves.cm import is_HCP
set_random_seed(297388353221545796156853787333338705098)
is_HCP(hilbert_class_polynomial(-55))

Currently error.

This was introduced in the PR #37170 because in the refactoring assume_distinct_deg worked as intended where as the older function seemed to simply always find roots the slow way regardless of how the function was called.

Expected Behavior

The function should work

Actual Behavior

The function does not work

Additional Information

This has been fixed in #37443. The only thing needed to do was to use the polynomial r, as intended, instead of the polynomial fp. This oversight lived in the code until recently purely because any_root() behaved weirdly before.

Environment

- **Sage Version**: SageMath version 10.3.beta8, Release Date: 2024-02-13

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
vbraun pushed a commit to vbraun/sage that referenced this issue Feb 28, 2024
    
This pull request aims to fix some issues which came with the
refactoring of `any_root()` in
sagemath#37170.

I am afraid that apart from a little more cleaning up and verbose
comments, this "fix" really relies on `f.roots()` rather than
`f.any_root()` when roots over extension fields are computed.

The core problem currently is that the proper workflow should be the
following:

- Given a polynomial $f$, find an irreducible factor of smallest degree.
When `ring` is `None` and `degree` is `None`, only linear factors are
searched for. This code is unchanged and works fast.
- When `degree` is `None` and `ring` is not `None`, the old code used to
perform `f.change_ring(ring).any_root()` and look for a linear factor in
`ring`. The issue is when `ring` is a field with a large degree,
arithmetic in this ring is very slow and root finding is very slow.
- When `degree` is not `None` then an irreducible of degree `degree` is
searched for, $f$, and then a root is found in an extension ring. This
is either the user supplied `ring`, or it is in the extension
`self.base_ring().extension(degree)`. Again, as this extension could be
large, this can be slow.

Now, the reason that there's a regression in speed, as explained in
sagemath#37359, is that the method before
refactoring simply called `f.roots(ring)[0]` when working for these
extensions. As the root finding is always performed by a C library this
is much faster than the python code which we have for `any_root()` and
so the more "pure" method I wrote simply is slower.

The real issue holding this method back is that if we are in some field
$GF(p^k)$ and `ring` is some extension $GF(p^{nk})$ then we are not
guaranteed a coercion between these rings with the current coercion
system. Furthermore, if we extend the base to some $GF(p^{dk})$ with
$dk$ dividing $nk$ we also have no certainty of a coercion from this
minimal extension to the user supplied ring.

As a result, a "good" fix is delayed until we figure out finite field
coercion. **Issue to come**.

Because of all of this, this PR goes back to the old behaviour, while
keeping the refactored and easier to read code. I hope one day in the
future to fix this.

Fixes sagemath#37359
Fixes sagemath#37442
Fixes sagemath#37445
Fixes sagemath#37471

**EDIT**

I have labeled this as critical as the slow down in the most extreme
cases causes code to complete hang when looking for a root. See for
example: sagemath#37442. I do not want
sagemath#37170 to be merged into 10.3
without this patch.

Additionally, a typo meant that a bug was introduced in the CZ
splitting. This has also been fixed.

Also, also in the refactoring the function behaved as intended (rather
than the strange behaviour from before) which exposed an issue
sagemath#37471 which I have fixed.
    
URL: sagemath#37443
Reported by: Giacomo Pope
Reviewer(s): Giacomo Pope, grhkm21, Lorenz Panny, Travis Scrimshaw
@vbraun vbraun closed this as completed in 7052c25 Feb 29, 2024
@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 a pull request may close this issue.

3 participants