Transpose elements in copy_transpose!

[maltezfaria]

Fixes JuliaLang/LinearAlgebra.jl#1033

Notes

• I had a hard time coming up with a self-contained test to cover this bug, as it only happens for elements which are both bitstype and for which transpose(x) != x. Not sure how to go about it. If we test copy_transpose! directly (and through the matrix multiplication), testing is straightforward.
• With Reduce compile time for generic matmatmul #52038 this is even more of a corner case since copy_transpose! is no longer used in the generic matrix multiplication. Still, I thought maybe it would still be worth fixing.

[dkarrasch]

For a test, you could call it directly, instead of the indirect path via generic_mul!. In case somebody removes the non-exported function later, they would simply remove the corresponding test as well.

[dkarrasch]

You could add a testset to the end of the transpose.jl test file, perhaps like this:

@testset "copy_transpose!" begin
    A = [randn(2,3) for _ in 1:2, _ in 1:3]
    At = copy(transpose(A))
    B = zero.(At)
    LinearAlgebra.copy_transpose!(B, axes(B, 1), axes(B, 2), A, axes(A, 1), axes(A, 2))
    @test B == At
end

[maltezfaria]

Thanks for the suggestion. I added a test and rebased it.

[dkarrasch]

CI looks horrible, but it seems all unrelated, and the change is small enough. So, I'd say this is "ready for review" and merge.

[maltezfaria]

CI looks horrible, but it seems all unrelated, and the change is small enough. So, I'd say this is "ready for review" and merge.

Thanks! Indeed, the changes are very simple.

As a general question, for PRs in Julia should I have master as the base (which evolves super quickly, thus requiring constant rebase), or should I branch off e.g. the newest tag (where I suppose the CI works)?

[dkarrasch]

Yes, master should be the base, but you don't need to rebase all the time. It's up to the people with merge rights whether they rebase before they merge a PR. But that mostly concerns older PRs. This one, for instance, has a very limited range of action, and doesn't interfere with all the other ongoing changes. In such a case, constant rebase would just add to the CI load, which is already high, with little gain in safety, if any.

[dkarrasch]

Thank you for reporting the issue and getting the fix started! Welcome to the Julia community @maltezfaria.

[maltezfaria]

Yes, master should be the base, but you don't need to rebase all the time. It's up to the people with merge rights whether they rebase before they merge a PR. But that mostly concerns older PRs. This one, for instance, has a very limited range of action, and doesn't interfere with all the other ongoing changes. In such a case, constant rebase would just add to the CI load, which is already high, with little gain in safety, if any.

That makes sense, thanks for the review and for taking the time to answer!