Skip to content

Fix extend handling in FiniteField_givaroElement.sqrt - #42720

Open
bhuvan-somisetty wants to merge 2 commits into
sagemath:developfrom
bhuvan-somisetty:fix-givaro-sqrt-extend
Open

Fix extend handling in FiniteField_givaroElement.sqrt#42720
bhuvan-somisetty wants to merge 2 commits into
sagemath:developfrom
bhuvan-somisetty:fix-givaro-sqrt-extend

Conversation

@bhuvan-somisetty

@bhuvan-somisetty bhuvan-somisetty commented Aug 23, 2026

Copy link
Copy Markdown

Description

Fixes #42719

In src/sage/rings/finite_rings/element_givaro.pyx, FiniteField_givaroElement.sqrt(extend=True, all=True) previously returned [] (empty list) on non-squares instead of raising NotImplementedError.

This occurred because the if all: check returned [] immediately when self.is_square() was False, completely bypassing the elif extend: raise NotImplementedError branch. As a result, extend=True was silently ignored whenever all=True, falsely claiming the element had no square roots in any extension field and violating the API contract.

Proposed Solution

  1. In FiniteField_givaroElement.sqrt(), check if extend: raise NotImplementedError upfront before checking all or evaluating base-field squareness (matching element_pari_ffelt.pyx and element_base.pyx).
  2. Correct the docstring default parameter note for extend from (default: True) to (default: False) to match the method signature.
  3. Add regression doctests for all combinations of (extend, all) for both squares and non-squares.

Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

Dependencies

None.

Signed-off-by: bhuvan-somisetty somisettybhuvan5@gmail.com

Fixes sagemath#42719

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
@cxzhong

cxzhong commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

please rebase to the lastest develop

@cxzhong

cxzhong commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Thanks for fixing the non-square all=True case from #42719. I think the new guard is too broad, though:

if extend:
    raise NotImplementedError

This also rejects elements whose roots already lie in the base field. For example, before this change, GF(9).one().sqrt(extend=True) returns 1, and GF(9).one().sqrt(extend=True, all=True) returns [1, 2]; after this change both raise NotImplementedError. The method documents extend=True as allowing an extension if necessary, so square elements should not require extension support.

Could you keep the existing elif extend: in the single-root path and add the exception only to the non-square all=True path?

if all:
    if self.is_square():
        a = self.sqrt()
        return [a, -a] if -a != a else [a]
    if extend:
        raise NotImplementedError
    return []

Please also add explicit extend=True tests for a square element, and ideally for characteristic 2, where every element is a square.

Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
@bhuvan-somisetty

Copy link
Copy Markdown
Author

Good catch, thank you! I've updated the logic so extend=True is allowed on elements whose roots already lie in the base field, and only raises NotImplementedError on non-squares. I also added doctests for square elements with extend=True as well as characteristic 2 fields.

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.

FiniteField_givaroElement.sqrt(extend=True, all=True) silently returns [] on non-squares instead of raising NotImplementedError

2 participants