Replace Val-types by singleton types in lu and qr

[dkarrasch]

This is a rebase of #25303. The first commit doesn't change the tests functionally and thereby proves that this is non-breaking. Seems like all @inlines in the qr code are necessary for type stability. If we go for it, we'll need tests for the new argument and a NEWS announcement.

Closes #25303, closes JuliaLang/LinearAlgebra.jl#476.

[simeonschaub]

Instead of forcing more aggressive inlining, it might be better to use just @aggressive_constprop instead, so we don't generate excessive code.

[dkarrasch]

Instead of forcing more aggressive inlining, it might be better to use just @aggressive_constprop instead, so we don't generate excessive code.

I didn't know about it, I'll try that out.

[dkarrasch]

This is ready for a first rough "review". Currently, this is "just" adding new accepted keyword values. Would we actively deprecate the old Val-type-based arguments, or let it go and try to remember to deprecate this as we are approaching v2.0? @andreasnoack @ViralBShah @stevengj may have some thoughts here.

[ViralBShah]

Maybe we create a label - 2.0-deprecation so that whenever we get to 2.0, we get all those things out.

@StefanKarpinski

[StefanKarpinski]

Deprecations are already off by default, so we can make something a deprecation already. For things that we want to change before 2.0, that's what the 2.0 milestone is for. I also keep a document with a list of things (linking to issues where appropriate). That's handy because sometimes a change is too small or vague to create an issue for but we still want to remember it.

[dkarrasch]

I have now updated the tests and deprecated the Val-type-based methods.

[dkarrasch]

Tests pass, this is ready for review.

[dkarrasch]

Anyone who wants to take a look? Since the old way of calling the decomposition functions continues to work, I guess the main issue to check is whether the NEWS entry is helpful and explicit enough.

[andreasnoack]

This is generally a great change that makes it much clearer which pivoting strategy is being used. However, I'm wondering if we should use empty structs instead of symbols here since we already to structs for selecting algorithm in SVD, ref

julia/stdlib/LinearAlgebra/src/LinearAlgebra.jl

Lines 163 to 165 in bb5b98e

	abstract type Algorithm end
	struct DivideAndConquer <: Algorithm end
	struct QRIteration <: Algorithm end

.

[dkarrasch]

That sounds like a good idea, since that no longer depends on constant propagation.

[dkarrasch]

Perhaps some input/feedback on the naming would be helpful. The SVD algorithms are not exported, but I chose to export the new PivotingStrategys.

[andreasnoack]

The SVD algorithms are not exported

It's actually one of the main issues with using structs: it seems excessive to export them when they are only used for dispatch in a few functions but it also feels wrong to qualify them. I think I prefer for exporting though.

I think the names are okay but I generally prefer to write out the words at the cost of some slightly longer names, i.e. something like ColumnNorm, RowMaximum (should it be RowMaximizer?).

[dkarrasch]

For the time being, I went with RowMaximum, because that's what the strategy is based on (similar to ColumnNorm), not referring to which pivot is chosen.

[dkarrasch]

Gentle bump.

[andreasvarga]

Sorry for interferring in this discussion, but because these changes (especially those in qr!) heavily affect my packages MatrixPencils and DescriptorSystems, I wonder if the recent failures of tests on the nightly version have to do with these changes. And, sorry for my ignorance, are the expected benefits of these changes somewhere discussed more in depth? For example, could I hope for shorter compilation times for the affected functions?

[dkarrasch]

This PR only renames Val{true/false} to more verbose types, specifically targetted to either lu or qr, which is the main benefit. The old signatures remain valid, but lead to deprecation warnings, as you can see in your CI runs. The algorithms underneath have not changed at all. From a quick glance, I don't think the failures on nightly are related to this change: the errors occur in "safe distance" to the deprecation warnings, as if the qr call worked ok, and the error occurred in a different test later.

[dlfivefifty]

What about cholesky?

[dkarrasch]

I was wondering the same recently after getting lost in the lu/cholesky/qr files when I still saw some Vals. How would you name the PivotingStrategy here?

[dlfivefifty]

It's "complete pivoting"

http://www.netlib.org/lapack/explore-html/da/dba/group__double_o_t_h_e_rcomputational_ga31cdc13a7f4ad687f4aefebff870e1cc.html

I believe this is equivalent to ColumnNorm() but also (due to symmetry) RowNorm(), which is currently not defined.

(I have a need for RowNorm() and ColumnMaximum() in MatrixFactorizations.jl but can add that there)

[dlfivefifty]

Actually It pivots the largest value to the diagonal, so maybe that's RowMaximum()?