Add NEWS entry for adjtrans of Triangular types

[dkarrasch]

This is a follow up to #38168, adding a NEWS entry, removing dead/commented out code, and adding type tests.

Ping @palday. 😉

Closes JuliaLang/LinearAlgebra.jl#783.

[palday]

I'll review tomorrow -- I need to think a little bit about whether the tests are covering all the cases I can think of and don't have the mental capacity today.

The NEWS looks good.

[dkarrasch]

The tests don't check the type explicitly, but only consistency of the type name (not the parameters) between adjoint! and adjoint. I was too lazy to write some loop with complementary pairs of types etc. Arguably, this consistency can be considered as the "bug fix" part of the original PR.

[dkarrasch]

While this doesn't have new code, can we still launch a package evaluation, to see who to inform about the change in #38168? @StefanKarpinski @ViralBShah

[ViralBShah]

Yes, we can. Perhaps @maleadt can tell us how to trigger PkgEval on before and after #38168.

[palday]

The tests don't check the type explicitly, but only consistency of the type name (not the parameters) between adjoint! and adjoint. I was too lazy to write some loop with complementary pairs of types etc.

I get that. 😄

Arguably, this consistency can be considered as the "bug fix" part of the original PR.

Yep, seeing it like this I can understand the motivation of the original change better. The actual order of the types seems arbitrary-ish to me (or rather I can see arguments for both orders), but consistency is important.

[maleadt]

PkgEval running here: 4704efd#commitcomment-44115567

[dkarrasch]

Thanks @maleadt! AFAICT, the MixedModels.jl failure is the only one induced by the original PR.

[ViralBShah]

Results of the PkgEval run- https://github.com/JuliaCI/NanosoldierReports/blob/master/pkgeval/by_hash/4704efd_vs_19506f7/report.md

[ararslan]

AFAICT, the MixedModels.jl failure is the only one induced by the original PR.

The ChainRules failure is likely related, as it relies heavily on specialization with LinearAlgebra types.

[dkarrasch]

AFAICT, the MixedModels.jl failure is the only one induced by the original PR.

The ChainRules failure is likely related, as it relies heavily on specialization with LinearAlgebra types.

I couldn't even find the term Triangular in their code base. But let's ask @oxinabox.

EDIT: I misremembered. I think I didn't even check the code of ChainRules.jl, because of the unrelated error, see #38398 (comment).

[oxinabox]

I am not 100% clear on the nature of this change

ChainRules uses Triangular matrixes in
https://github.com/JuliaDiff/ChainRules.jl/blob/d40e3e174524a6f40e40b114e6e89d3bd413c6d3/src/rulesets/LinearAlgebra/structured.jl
and it calls adjoint and transpose on them.
Quite possibly it should just be calling conj on them.
I would need to think a bit I guess.

[dkarrasch]

There is only one error in ChainRules.jl:

3-arg dot, Array{ComplexF64}: Test Failed at /home/pkgeval/.julia/packages/ChainRulesTestUtils/wjZiE/src/check_result.jl:18
  Expression: isapprox(add!!(acc_mutated, val), acc + val; kwargs...)
   Evaluated: isapprox(ComplexF64[-1.1701131866419396 - 3.79687561601824im, -0.36934589076936375 - 1.7677498974545742im, 2.9343957655656716 + NaN*im], ComplexF64[-1.1701131866419396 - 3.79687561601824im, -0.36934589076936375 - 1.7677498974545742im, 2.9343957655656716 + NaN*im]; rtol = 1.0e-9, atol = 1.0e-9)

It looks to me like an accuracy issue, not a deep type-related failure. What is potentially critical for downstream packages is when they methods with signatures à la Adjoint{<:Any,<:LowerTriangular}, because these types are no longer realized (instead UpperTriangular{<:Any,<:Adjoint}). This is what happened in MixedModels.jl.

[oxinabox]

Oh that. That is not related to this.
It's a no deterministic fault because some tests just apparently assumed for y=similar(x) that isapprox(y, y)
It was fixed the other day
JuliaDiff/ChainRules.jl#304

[dkarrasch]

Are we happy with this? Ready to go?