proper diagonal in copytri! (fix #30055)

[KlausC]

fix of #30055

[KlausC]

It's 4 weeks now and doesn't get better by waiting.... :-)

[StefanKarpinski]

bump: @andreasnoack & @KristofferC

[andreasnoack]

I don't think copytri! is the right place to fix this. That function is correct as it is. It just leaves the diagonal which I think is fine. Instead, I'd modify

julia/stdlib/LinearAlgebra/src/triangular.jl

Lines 349 to 356 in dffe119

	transpose!(A::LowerTriangular) = UpperTriangular(copytri!(A.data, 'L'))
	transpose!(A::UnitLowerTriangular) = UnitUpperTriangular(copytri!(A.data, 'L'))
	transpose!(A::UpperTriangular) = LowerTriangular(copytri!(A.data, 'U'))
	transpose!(A::UnitUpperTriangular) = UnitLowerTriangular(copytri!(A.data, 'U'))
	adjoint!(A::LowerTriangular) = UpperTriangular(copytri!(A.data, 'L' , true))
	adjoint!(A::UnitLowerTriangular) = UnitUpperTriangular(copytri!(A.data, 'L' , true))
	adjoint!(A::UpperTriangular) = LowerTriangular(copytri!(A.data, 'U' , true))
	adjoint!(A::UnitUpperTriangular) = UnitLowerTriangular(copytri!(A.data, 'U' , true))

to also take care of the diagonal. Notice that also the transpose case is wrong for matrix elements.

[andreasnoack]

Alternatively, you could add an argument to copytri! to include the diagonal in the existing loop. That might actually be cleaner and it would automatically fix the transpose case as well.

[codecov-io]

Codecov Report

Merging #30066 into master will increase coverage by 15.7%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #30066      +/-   ##
==========================================
+ Coverage   41.31%   57.02%   +15.7%     
==========================================
  Files         349      503     +154     
  Lines       62360    87313   +24953     
==========================================
+ Hits        25767    49794   +24027     
- Misses      36593    37519     +926

Impacted Files	Coverage Δ
base/event.jl	72.92% <0%> (-14.71%)	⬇️
base/char.jl	86.56% <0%> (-6.83%)	⬇️
base/logging.jl	82.85% <0%> (-5.53%)	⬇️
base/intfuncs.jl	90.25% <0%> (-4.21%)	⬇️
base/channels.jl	88.7% <0%> (-4.15%)	⬇️
base/process.jl	81.22% <0%> (-2.99%)	⬇️
base/strings/substring.jl	94.84% <0%> (-2.94%)	⬇️
base/initdefs.jl	52.72% <0%> (-2.28%)	⬇️
base/bitarray.jl	88.91% <0%> (-2.21%)	⬇️
base/reduce.jl	89.44% <0%> (-2.09%)	⬇️
... and 236 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40cdae2...fa3d7ad. Read the comment docs.

[KlausC]

Alternatively, you could add an argument to copytri! to include the diagonal in the existing loop. That might actually be cleaner and it would automatically fix the transpose case as well.

Done.
A variant of copytri! has now Valarguments to avoid runtime checks on the different arguments.
Also a test case for transpose was added.

[andreasnoack]

A variant of copytri! has now Valarguments to avoid runtime checks on the different arguments.

This should no longer be necessary with constant propagation. Might need to be forcibly inlined, though.

[KlausC]

This should no longer be necessary with constant propagation. Might need to be forcibly inlined, though.

In v1.0.3 the output of @code_warntype is approximately twice of the current. uplo is typically a variable, not a constant, when calling copytri!.

[andreasnoack]

In v1.0.3 the output of @code_warntype is approximately twice of the current.

It can be a bit tricky to get correct output from constant propagation with @code_warntype. How did you invoke it? You should try wrapping the call in a function with constant arguments to copytri!.

uplo is typically a variable, not a constant, when calling copytri!.

The two other are and uplo is the outer branch so I don't think that one is critical.

[KlausC]

You should try wrapping the call in a function with constant arguments to copytri!

I did @code_warntype LinearAlgebra.copytri!(B, 'U', false). That showed the code with runtume checks for everything.

Now I tried

julia> foo(B) = LinearAlgebra.copytri!(B, 'U', false)
foo (generic function with 1 method)

@code_warntype foo(B)
Body::Array{Int64,2}
1 1 ─ %1 = LinearAlgebra.copytri!::Core.Compiler.Const(LinearAlgebra.copytri!, false)         │
  │   %2 = invoke %1(_2::Array{Int64,2}, 'U'::Char, false::Bool)::Array{Int64,2}              │
  └──      return %2 

That does not show any code from inside copytri!, because it is not inlined.

[andreasnoack]

Just tried it out locally and the propagation works fine if you just add @inline.

[KlausC]

Ok. I have now
@inline copytri!(...., uplo::AbstractChar, conjugate::Bool=false, diag::Bool=false)

[andreasnoack]

Great. Thanks for fixing this. We can merge once tests pass.

[KlausC]

broken windows test run seems unrelated. All LinearAlgebra tests passed. maybe time-out afterwards.

[KlausC]

thanks for the review.