Condense doc for is_square into single docstring #2186
Conversation
|
Bother! I meant to choose draft PR. Oh well... |
|
Just click on "Convert to draft"... |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2186 +/- ##
=======================================
Coverage 87.95% 87.95%
=======================================
Files 127 127
Lines 31791 31791
=======================================
+ Hits 27961 27962 +1
+ Misses 3830 3829 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Apparently already done. I'll un-draft-ify the PR.
src/MatRing.jl
Outdated
| # See generic documentation in NCRings.jl | ||
| is_square(a::MatRingElem) = true |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Function definition commented out and pushed. If the tests pass, I can delete the function definition.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
src/AbsSeries.jl
Outdated
| return q | ||
| end | ||
|
|
||
| # See generic documentation in NCRings.jl |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK - removed. Will push shortly.
Co-authored-by: Max Horn <max@quendi.de>
…ds has been eliminated
2 participants
i thought I had already made this into a draft PR