Implement Square Roots to FiniteFields Category

[Brian-Heckel]

Fixes #39688
Fixes #39798

Added the following methods to the finite field category

• quadratic_non_residue()
• square_root() and sqrt()
• _tonelli() and _cipolla(): the algorithms for finding a square root
• is_square() Moved is_square() and sqrt() methods from element.pyx to their proper categories.

Most profiling tests showed that Tonelli's algorithm outperforms Cipolla. However a profile with a Finite Field with order of a Cullen prime resulted in Cipolla outperforming Tonelli.

I'm running the meson build and currently cannot find the way to build the html documentation.

📝 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

[vincentmacri]

Brian did this work as part of an undergrad mentorship program at the University of Calgary, and I was the grad student mentoring him. I'll review this for mathematical correctness/style/documentation/etc., but since I worked closely with him on this I'd like a second person to review it as well (especially for the refactoring that was done to move the default square root implementation out of element.pyx and into the appropriate category).

[vincentmacri]

First pass, mostly style things and wording in the documentation.

[vincentmacri]

Docstring fixes

[vincentmacri]

Docstring formatting

[user202729]

Not completely related to this change:

@tobiasdiez another ruff failure here, even though the code is just moved from one file to another without any whitespace change. What's going on? (different rules are applied on .py and .pyx files, I guess? Is there a linter for pyx?)

[user202729]

do fix the lints.

Also, build the HTML files locally (the CI for HTML preview is currently broken) and check that they're rendered correctly. They're incorrect in many places.

Check some existing docstring for the formatting. I think first word should be in infinitive form, first sentence has full stop, and no bullet point in output if only one output. (Do verify that.)

Refer to #39798 .

First character of error message should not be capitalized.

What's the algorithm for computing square root in e.g. GF(5^2)? If it's also tonelli or cipolla, consider giving a different name for the sage implementation e.g. sage_tonelli to distinguish it.

[tobiasdiez]

Not completely related to this change:

@tobiasdiez another ruff failure here, even though the code is just moved from one file to another without any whitespace change. What's going on? (different rules are applied on .py and .pyx files, I guess? Is there a linter for pyx?)

Yes, ruff is not supporting cython.

[vincentmacri]

Docstrings, cleanup.

[vincentmacri]

do fix the lints.

Curious, did you approve the CI run? I don't have permission to approve CI runs was wondering who did (so I can ping them to do it again).

Also, build the HTML files locally (the CI for HTML preview is currently broken) and check that they're rendered correctly. They're incorrect in many places.

That's annoying that the CI is broken. The process to build the docs with Meson is not very well-documented (I should get around to filing a bug report about that eventually...), but I eventually figured it out and sent the instructions to Brian.

What's the algorithm for computing square root in e.g. GF(5^2)? If it's also tonelli or cipolla, consider giving a different name for the sage implementation e.g. sage_tonelli to distinguish it.

I'm not too sure what you mean by wanting to distinguish with the naming, but I don't think we need to worry about it. GF uses optimized implementations written in Cython, so the sqrt added here will not be called by an element of GF(q), only by elements of quotient rings that are known to be fields by Sage (or any other way one manages to construct something belonging to the FiniteFields category that isn't an instance of GF, but the only such way I know of is the polynomial quotient ring). Notably, the GF implementations do not have an algorithm option for their sqrt methods.

In terms of what algorithm GF uses, for GF(5^2) we use the Givaro library. For GF(p) we use Tonelli (might be a variant of Tonelli, not 100% sure). For GF(p^n) where p^n is large and n > 1 we use Pari. There might be other situations in which we do different things or use other libraries.

That said, I do think that at least for GF(p) when p is large and p - 1 is highly divisible by 2, the _cipolla function can outperform the Cython integer_mod Tonelli implementation. One such case we found was the Cullen prime p = 141 * 2^141 + 1. For future work (i.e. I consider out of scope for this PR) we might want to do some more detailed profiling on exactly when Cipolla outperforms Tonelli and either implement that in Cython and use it when p is large enough, or just call the Python _tonelli implementation added by this PR.

[vincentmacri]

Failures seem irrelevant. I'm going to try building locally.

[vincentmacri]

The failed CI runs work fine on my Fedora machine, so i think the failures are just the CI being flaky. (I didn't test the PDF build since I think that's broken already, and I didn't test Windows because I don't have a Windows machine.)

@fchapoton @user202729 do either of you want to set this to positive review?

[user202729]

currently for Zmod(p) the method is named quadratic_nonresidue, not quadratic_non_residue. I think it's best to stay consistent.

[vincentmacri]

Not sure why GitHub says I'm requesting changes. I agree with the other reviewer's suggestions and do not have additional changes that I want to request.

[vincentmacri]

Not sure why it's still needed.

It seems to work fine now.

In case this comes up again in the future, I think it was needed before because @Brian-Heckel was added to the SageMath organization after the commit was pushed. It seems that adding someone to the SageMath organization does not automatically approve pending CI runs, but does automatically approve future CI runs.

[vincentmacri]

No further comments (does this make GitHub stop complaining about me requesting changes?)

[dimpase]

would you edit the labels set 'positive review" one?

[vincentmacri]

would you edit the labels set 'positive review" one?

As I said (much) earlier in this thread, I think I worked too closely with Brian to be comfortable giving this a positive review myself. While Brian did write the code himself, I think it's a bit of a conflict of interest for me to approve it alone.

Brian did this work as part of an undergrad mentorship program at the University of Calgary, and I was the grad student mentoring him. I'll review this for mathematical correctness/style/documentation/etc., but since I worked closely with him on this I'd like a second person to review it as well (especially for the refactoring that was done to move the default square root implementation out of element.pyx and into the appropriate category).

I'm planning to use a different workflow for these situations in the future to make this process a bit faster/less confusing.

[user202729]

okay, unless @fchapoton has other comments.
if there's some code that does (<CommutativeRingElement> x).is_square() then it would be slowed down slightly, but likely unimportant.

[vincentmacri]

if there's some code that does (<CommutativeRingElement> x).is_square() then it would be slowed down slightly, but likely unimportant.

The old is_square in CommutativeRingElement raised NotImplementedError, so I don't think anything is using it.

In theory this could slow down calls to (<CommutativeRingElement> x).sqrt() but it should be minimal as sqrt doesn't contain any performance-critical code, it's just a wrapper around other functions. Also a lot of rings override sqrt anyway.

Out of scope for this PR, but Brian and I did talk about the possibility of implementing some of the category methods in Cython, I posted about it on sage-devel a while ago but didn't get any response. If you have any ideas/thoughts/knowledge on that feel free to reply there.

[Brian-Heckel]

Thanks for the review @user202729!

[user202729]

In theory this could slow down calls to (<CommutativeRingElement> x).sqrt() but it should be minimal as sqrt doesn't contain any performance-critical code, it's just a wrapper around other functions. Also a lot of rings override sqrt anyway.

Point is cython generates a vtable for each declared method of cdef class, so if something overrides it, the overridden method can be quickly called (just lookup the function pointer from the vtable and call it, instead of going through Python getattr) if you know the object is an instance of the parent class.

[vincentmacri]

Point is cython generates a vtable for each declared method of cdef class, so if something overrides it, the overridden method can be quickly called (just lookup the function pointer from the vtable and call it, instead of going through Python getattr) if you know the object is an instance of the parent class.

As I understand it (I may be wrong), if a method is implemented in a class then it replaces (not overrides) the method from the category framework. So I think anything that implements is_square in Cython would still use the Cython optimizations you're talking about.

[user202729]

As I understand it (I may be wrong), if a method is implemented in a class then it replaces (not overrides) the method from the category framework.

Yes.

So I think anything that implements is_square in Cython would still use the Cython optimizations you're talking about.

No, Cython code that does x.is_square() where x is known to have type CommutativeRingElement will do a vtable lookup only if CommutativeRingElement is an extension class and defines a method named is_square.

Out of scope for this PR, but Brian and I did talk about the possibility of implementing some of the category methods in Cython, I posted about it on sage-devel a while ago but didn't get any response. If you have any ideas/thoughts/knowledge on that feel free to reply there.

Speaking of this, there are some small issues (Python-implemented method "cannot" override Cython-implemented method), but in principle they should be all solvable. In any case, if the methods themselves are expensive (rather than you want to optimize the overhead of calling a Python method), an easy way out is to implement a helper library in pyx, then make the category method call that.

I haven't looked at the situation too closely, however.

[vincentmacri]

No, Cython code that does x.is_square() where x is known to have type CommutativeRingElement will do a vtable lookup only if CommutativeRingElement is an extension class and defines a method named is_square.

I'm don't quite understand what you mean. Here's an example of what I meant. integer_mod.pyx defines is_square. Wouldn't this replace the Python is_square from the finite field category and thus use the lookup you're talking about?

In any case, if the methods themselves are expensive (rather than you want to optimize the overhead of calling a Python method),

I think it's more an issue of the function call overhead. At least in this PR the underlying arithmetic is still handled by Cython or compiled libraries.

an easy way out is to implement a helper library in pyx, then make the category method call that.
I haven't looked at the situation too closely, however.

I've considered that too, but was surprised to see there is very little Cython code in the category framework. Not sure if this is because of technical limitations that I would encounter if I tried or if it's just because nobody has gotten around to it yet.

[user202729]

Using the cpdef keyword instead of cdef, a Python wrapper is also created, so that the function is available both from Cython (fast, passing typed values directly) and from Python (wrapping values in Python objects). In fact, cpdef does not just provide a Python wrapper, it also installs logic to allow the method to be overridden by python methods, even when called from within cython. This does add a tiny overhead compared to cdef methods.

I stand corrected.

There are some code in the category framework that manually does what this does automatically e.g.

        try:
            python_op = (<object>self)._add_
        except AttributeError:
            raise bin_op_exception('+', self, other)
        else:
            return python_op(other)

in element.pyx. I thought this was because it isn't automatically supported.