Condense doc for is_square into single docstring #2186

JohnAAbbott · 2025-10-15T08:23:58Z

i thought I had already made this into a draft PR

JohnAAbbott · 2025-10-15T08:27:08Z

Bother! I meant to choose draft PR. Oh well...
I made a change to Float.jl because the existing code seemed to be incorrect (but I did not add a test).
I note also that the code in Rational.jl is correct only under certain (reasonable) assumptions: e.g. that the denominator must be positive, and numerator and denominator coprime.

fingolfin · 2025-10-15T08:36:00Z

Just click on "Convert to draft"...

codecov · 2025-10-15T08:44:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.00%. Comparing base (2980528) to head (0d87449).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2186      +/-   ##
==========================================
+ Coverage   87.95%   88.00%   +0.04%     
==========================================
  Files         127      127              
  Lines       31791    31769      -22     
==========================================
- Hits        27961    27957       -4     
+ Misses       3830     3812      -18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

fingolfin · 2025-10-15T10:57:25Z

src/NCRings.jl

+@doc raw"""
+    is_square(a::T)  where {T <: NCRingElement}
+    is_square(M::MatElem)
+    is_square(M::MatRingElem)
+
+There are two distinct situations:
+(1) if the argument is matrix then check whether the matrix **shape** is square
+(2) if the argument is not a matrix then check whether the **value** is a square (in the same ring)
+"""
+function is_square end


IMHO this really should be two docstrings:

one for is_square(a::NCRingElement)

one for is_square(M::MatElem)

Both then can be included in different parts of the manual (one in the ring interface section, one in matrix interface section).

ping @JohnAAbbott

Apparently already done. I'll un-draft-ify the PR.

…d files

fingolfin · 2025-11-03T14:08:16Z

src/MatRing.jl

+# See generic documentation in NCRings.jl
 is_square(a::MatRingElem) = true


Ah, but this is wrong now: according to what we discussed, MatRingElem are always square matrices "on the inside", but since they are NCRingElem, the meaning of is_square here now should be "is there some b such that b^2 == a ?", and that won't always be true.

So this needs to be removed, including the block comment mentioning is_square above it.

This is a bit confusing. Your suggestion is that for matrices (not of type MatRingElem) the function is_square checks the shape of the matrix. But if the matrix happens to be of type MatRingElem then is_square should behave differently? Well, of course, if one knows that the matrix is of type MatRingElem then there's no point in checking whether it is square...
So should is_square applied to a MatRingElem produce a not implemented error?

A MatRingElem is not a matrix, i.e., not a MatElem (similar for MatrixGroupElem). Rather it is a NCRingElem. That was a major point in this discussion and explicitly discussed during the relevant triage discussion.

But "luckily" we have no general algorithm for computing square roots of matrices (I think) so by simply not implementing this method for MatRingElem all is well

Function definition commented out and pushed. If the tests pass, I can delete the function definition.

There's a catch! We have MatrixElem which is an alias for Union{MatElem, MatRingElem}. Then a method such as det(::MatrixElem) needs to check that its argument has square shape. What to do?

The proper clean solution should be to resolve #1955.

Then det(::MatrixElem) can be changed into det(::MatElem), and a new method det(x::MatRingElem) = det(matrix(x)) can be added (where matrix(x::MatRingElem) returns the matrix wrapped by x).

fingolfin · 2025-11-03T14:09:18Z

src/AbsSeries.jl

   return q
 end

+# See generic documentation in NCRings.jl


While I understand the intention, I note that we do not have such comments in front of the many other methods that implement ring interfaces, so it seems odd to have the comment added to just is_square. I'd rather not add this everywhere at all.

OK - removed. Will push shortly.

src/generic/Matrix.jl

Co-authored-by: Max Horn <max@quendi.de>

…ds has been eliminated

src/MatRing.jl

fingolfin · 2025-11-19T00:20:48Z

docs/src/fraction.md

 ```@docs
 Base.sqrt(::FracElem{T}) where {T <: RingElem}
 ```


By the looks of it, the same treatment should be extended to the Base.sqrt methods -- and then perhaps instead of removing the sections here outright, we could something like this:

Suggested change

```@docs

Base.sqrt(::FracElem{T}) where {T <: RingElem}

```

Methods for `is_square` and `sqrt` are provided for inputs of type `FracElem`.

fingolfin

Actually I just realized that is_square is now missing from the manual which also isn't good...

fingolfin · 2025-11-19T00:25:11Z

src/NCRings.jl

+@doc raw"""
+    is_square(a::T)  where {T <: NCRingElement}
+
+Return `true` iff `a` is the square of a value in its own ring.
+See also `is_square(M::MatElem)` which tests whether a matrix has square shape.
+"""
+function is_square end


Actually, this docstring should be added to the manual. For now the "most obvious" spot would be in docs/src/ring_interface.md under "Optional functionality" (I am no fan of the current split between ring.md and ring_interface.md, I can never tell in which file to look -- but that should not matter here, we need to clean that up in a separate effort).

SoongNoonien · 2025-11-20T11:00:06Z

src/NCRings.jl



+@doc raw"""
+    is_square(a::T)  where {T <: NCRingElement}


Suggested change

is_square(a::T) where {T <: NCRingElement}

is_square(a::NCRingElement)

Of course this is a very minor complaint. But at least to me these function signatures are confusing, I always start wondering if there is a reason to write it with the type parameter. And I don't see a reason to write it this way in a docstring.

fingolfin · 2025-11-26T13:08:55Z

OK so the one missing thing here is that the docstring is_square should be included into docs/src/ring.md or docs/src/ring_interface.md (so a section with a @docs block containing it should be added).

Afterwards we could merge it. And perhaps @varuntrehan7 could then make a similar PR to this one for sqrt. I think there are a bunch of other functions that could stand a similar treatment.

fingolfin · 2025-11-26T14:02:06Z

So, slight change of plan: we'll merge this now, so that Varun can begin working on the sqrt bit, and I'll open an issue about adding the is_square docstring into the manua,

Condense doc for is_square into single docstring

746ec2b

JohnAAbbott marked this pull request as draft October 15, 2025 09:26

lgoettgens mentioned this pull request Oct 15, 2025

[quibble/documentation] is_square for matrices (esp. ZZMatrix) oscar-system/Oscar.jl#5441

Closed

fingolfin reviewed Oct 15, 2025

View reviewed changes

Added test for is_square of float zero; removed some docblocks from m…

ddd7300

…d files

JohnAAbbott marked this pull request as ready for review October 29, 2025 09:20

fingolfin reviewed Nov 3, 2025

View reviewed changes

src/generic/Matrix.jl Outdated Show resolved Hide resolved

JohnAAbbott and others added 3 commits November 3, 2025 16:42

Update src/generic/Matrix.jl

707b4cc

Co-authored-by: Max Horn <max@quendi.de>

Removed xref comments pointing to generic documentation for is_square

ea4d8b4

Delete tests of fn is_square applied to MatRingElem, since that metho…

c049a50

…ds has been eliminated

fingolfin added the triage label Nov 5, 2025

fingolfin reviewed Nov 12, 2025

View reviewed changes

src/MatRing.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

0d87449

fingolfin approved these changes Nov 19, 2025

View reviewed changes

fingolfin reviewed Nov 19, 2025

View reviewed changes

fingolfin requested changes Nov 19, 2025

View reviewed changes

SoongNoonien reviewed Nov 20, 2025

View reviewed changes

fingolfin approved these changes Nov 26, 2025

View reviewed changes

fingolfin merged commit 1a7d93c into Nemocas:master Nov 26, 2025
23 checks passed

fingolfin mentioned this pull request Nov 26, 2025

Add the is_square docstring to the manual #2222

Open

JohnAAbbott deleted the JAA/doc-for-is-square branch November 27, 2025 12:37

lgoettgens removed the triage label Nov 27, 2025

lgoettgens mentioned this pull request Dec 1, 2025

condense doc for sqrt into single docstring #2226

Draft

		# See generic documentation in NCRings.jl
		is_square(a::MatRingElem) = true

	is_square(a::T) where {T <: NCRingElement}
	is_square(a::NCRingElement)

Condense doc for is_square into single docstring #2186

Condense doc for is_square into single docstring #2186

Uh oh!

Conversation

JohnAAbbott commented Oct 15, 2025

Uh oh!

JohnAAbbott commented Oct 15, 2025

Uh oh!

fingolfin commented Oct 15, 2025

Uh oh!

codecov bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Nov 26, 2025

Uh oh!

fingolfin commented Nov 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 15, 2025 •

edited

Loading