Enhance performance of generic transpose and transpose! methods, restrict in-place transpose! to square matrices

[fingolfin]

Also forbid in-place transpose! for non-square matrices.

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.33%. Comparing base (3306a9a) to head (2029fc6).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Matrix.jl	80.00%	3 Missing ⚠️
src/generic/MatRing.jl	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1937      +/-   ##
==========================================
+ Coverage   88.06%   88.33%   +0.27%     
==========================================
  Files         126      126              
  Lines       31736    31797      +61     
==========================================
+ Hits        27948    28088     +140     
+ Misses       3788     3709      -79     

☔ 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]

Tests fails because in Nemo some transpose! methods with just one argument only work on square matrices.

Perhaps that the right decision, and I should match it here, and just throw an error in that case, too? Perhaps a bit more helpful one, but an error nevertheless.

Or perhaps the Nemo methods should be made more general.

Any thoughts on this, anyone?

[thofma]

I never missed it for non-square matrices, but maybe @fieker has opinion/wishes?

[fieker]

if it was missing - how can there be a test for it? I never used inplace transpose on non-square matrices. i do understand that this is possible (in flint) and an interesting research problem....

[fingolfin]

Reviving this PR from 1 year ago:

The existing method for transpose! simply delegates to transpose:

src/Matrix.jl:1488:transpose!(A::MatrixElem) = transpose(A)

And there is an existing test which invokes transpose! on a non-square matrix:

  a = QQ[1 2 3; 4 5 6]
...
  b = transpose(a)
  @test AbstractAlgebra.transpose!(a) == b

So I have two choices here:

1. adjust the test to only invoke transpose! on a square matrix, and enforce that in the implementation
2. allow non-square matrices (as the implementation here currently does).

Personally I'd be perfectly happy with 1, if we can agree on it (the comments from a year ago suggest that might be the case)

[fingolfin]

This PR also adds a docstring for transpose! which of course should be adjusted depending on what we decide here.

It also inserts that docstring into the manual. Of course we also should agree on whether we want that (I don't see why not, but...)

[fingolfin]

I noticed that Nemo also explicitly supports non-square matrices, e.g. it has

transpose!(A::Union{ZZMatrix,QQMatrix}) = is_square(A) ? transpose!(A, A) : transpose(A)

On the other hand, it also errors out in a few cases, e.g.:

function transpose!(a::FqMatrix)
  !is_square(a) && error("Matrix must be a square matrix")
  @ccall libflint.fq_default_mat_transpose(a::Ref{FqMatrix}, a::Ref{FqMatrix}, base_ring(a)::Ref{FqField})::Nothing
end

[JohnAAbbott]

As a "naive" user I would wonder what transpose! might do to a non-square matrix if it doesn't give error. Are there other in-place operations which can change the shape of a matrix?
Presumably changing the code so that an error is triggered if the input is non-square would count as a breaking change (assuming the code currently doesn't give an error). That said, one could argue that changing the code to give an error where previously it did not (but should have done?) is making the code safer rather than a true breaking change: anyway any existing code using the "old" behaviour would get a helpful error message rather than mysteriously give a different result.
Since two important users/developers say that they have never needed the suspect behaviour, we could argue that this further justifies making the change.

[fingolfin]

I've modified the PR to prevent in-place transpose for non-square matrices. Let's see if it breaks anything in Nemo, Hecke or Oscar.

[fingolfin]

Are these being tested right now?

[fingolfin]

I think this is good to merge now, if someone would like to approve it ...

[thofma]

I think you have to merge ignoring the checks.

[thofma]

It was all green. Why the restart?

[lgoettgens]

It was all green. Why the restart?

I thought #2259 would resolve the CI issue. But I now noticed it is due to matching branches...
Let me just add my suggestion to not run CI another unnecessary time