Replace Val-types in cholesky[!] #41640

dkarrasch · 2021-07-19T15:13:14Z

Extends #40623 to cholesky. Addresses #40623 (comment).

stdlib/LinearAlgebra/src/cholesky.jl

andreasnoack · 2021-07-20T09:24:42Z

stdlib/LinearAlgebra/test/cholesky.jl

@@ -132,7 +132,7 @@ end

        #pivoted upper Cholesky
        if eltya != BigFloat
-            cpapd = cholesky(apdh, Val(true))
+            cpapd = cholesky(apdh, RowMaximum())


I'm wondering if we should maintain some tests that use cholesky(apdh, Val(true)) to ensure that the deprecations work and to avoid accidental breakage of later?

Suggested change

cpapd = cholesky(apdh, RowMaximum())

cpapd = cholesky(apdh, RowMaximum())

That makes the tests fail, see my first commit and its fix in the second.

Also, we don't have that for lu and qr. Seeing the few "fix breakage" PRs in downstream packages made me feel bad about deprecating these functions, though there were no objections against that. Maybe we shouldn't officially deprecate them but only redirect them and list those functions in the deprecated.jl list?

Surprisingly, in my local build I don't even get a deprecation warning.

That makes the tests fail

I see. So I guess CI runs with --depwarn=error?

Surprisingly, in my local build I don't even get a deprecation warning.

Do you run with --depwarn=yes?

I just start the locally built Julia and then using LinearAlgebra; A = rand(3,3); A = A'A; cholesky(A, Val(true)). So, I wasn't referring to the tests, just to "normal usage".

Yeah but the default now is --depwarn=no so users won't see these unless you set --depwarn=yes on startup.

thchr · 2021-07-20T15:15:31Z

Just for the record: the changes in #40623 did break code elsewhere, e.g., JuliaArrays/StaticArrays.jl#931.
I suppose it's really kind of borderline because the breakage arose due to the introduction of ambiguity errors - but it definitely had an impact.

I think the ambiguity errors there arose from changing certain ::Union{Val{false}, Val{true}} signatures to (deprecated) methods with distinct Val{false} and Val{true} dispatch - but I'm not 100% sure (e.g., qr didn't have any Union dispatch before #40623, but still broke in StaticArrays after #40623).

AFAICT, there's no conversion of Union-style dispatches here - so it it might not be a problem - but I just wanted to note this effect in any case.

dkarrasch · 2021-07-21T07:07:48Z

Sorry to hear that. I'll run tests this time.

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2021-07-21T15:49:18Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

dkarrasch · 2021-07-21T16:18:45Z

AFAICT, there are 3 packages affected: GLM, NiceNumbers and SciMLBase. I wonder what happens if we don't export the "old signature". For instance, this works:

module MyTest

struct MyType end
export f
f(x, ::MyType) = 3x
@deprecate f(x, ::Val{false}=Val(false)) f(x, MyType()) false
end
using .MyTest
f(2, Val(false))
f(2, MyTest.MyType())

I wonder if that prevents ambiguity errors.

dkarrasch · 2021-07-21T16:21:09Z

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2021-07-21T22:28:51Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

dkarrasch · 2021-07-22T08:26:14Z

@nanosoldier runtests(["GLM", "NiceNumbers", "SciMLBase"], vs = ":master")

nanosoldier · 2021-07-22T10:30:25Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

dkarrasch · 2021-07-23T09:20:28Z

This PR makes GLM.jl (for a reason that I don't understand) and NiceNumbers.jl (which would be easy to fix) fail, but not SciMLBase.jl (which had ambiguity issues initially) anymore.

dkarrasch · 2021-07-23T09:29:42Z

stdlib/LinearAlgebra/src/cholesky.jl

-positive semi-definite matrix `A`. This is the return type of [`cholesky(_, Val(true))`](@ref),
+positive semi-definite matrix `A`. This is the return type of [`cholesky(_, ::RowMaximum)`](@ref),


The current cross reference doesn't work properly and links to the cholesky-docstring from SparseArrays.jl. Is this type-based reference better or how do I specify which docstring to link to? @fredrikekre

palday · 2021-08-04T16:08:03Z

@dkarrasch MixedModels did get test breakage from the qr change -- in part because of the change in warnings we got, which broke the tests checking the warnings. A quick look at the report from GLM suggests that this is also part of the problem.

cf. JuliaStats/MixedModels.jl#547

dkarrasch · 2021-08-06T11:41:16Z

Hm, could be that it's better not to deprecate, but to redirect then? I wasn't sure about that, but nobody told me to not deprecate. OTOH, some people seem to like these type-based changes. So, how do we proceed with maximum benefit and least annoyance?

palday · 2021-08-06T13:41:35Z

Ultimately, deprecation is just re-direction with a warning and it would be good to be consistent with qr. So I would say deprecate, but do PRs against GLM.jl and other packages with a wrapper like

@static if VERSION < v"1.8.0-DEV.xxxx" 
    pivoted_cholesky(A; kwargs...) = cholesky(A, Val(true); kwargs...)
else
    pivoted_cholesky(A; kwargs...) = cholesky(A, RowNorm(); kwargs...)
end

and replace all the calls in that package as appropriate. Then those tests don't break, users aren't annoyed by deprecation warnings coming from package code, and inlining should prevent any performance hit.

(If qr didn't already deprecate, then I would say just redirect and let inlining handle everything because a lot of users just wanted "pivoting" without thinking about the pivoting strategy.)

dkarrasch · 2021-12-09T15:19:45Z

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2021-12-09T22:26:45Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

palday · 2021-12-09T22:32:40Z

@dkarrasch I think the GLM failure is the one mentioned above.

dkarrasch · 2021-12-13T10:36:40Z

@dkarrasch I think the GLM failure is the one mentioned above.

I know, which is why GLM.jl already has a patch PR. 😉

dkarrasch · 2021-12-13T11:11:28Z

How do we proceed here? There are three PRs in the pipeline to update failing packages (out of those that currently pass their test suite). Shall we merge now, update (if necessary) the specific VERSION number in the accompanying PRs and see how it goes? There's one last question about how to fix the currently broken reference to the pivoted cholesky docstring. @fredrikekre Could you please take a look when you find the time?

palday · 2021-12-13T14:47:12Z

@dkarrasch Cool, thanks. I'll review the GLM.jl PR as soon as this one here lands (so tag me as a reviewer when that happens). 😄

dkarrasch · 2021-12-14T13:36:22Z

@nanosoldier runtests(["CMAEvolutionStrategy", "CryptoDashApp", "DCISolver", "DataFrames", "GraphQLClient", "Hecke", "ITensors", "JetPackWaveFD", "OptimKit", "PolaronPathIntegrals", "QuantumTomography", "TreeParzen"], vs = ":master")

nanosoldier · 2021-12-14T14:50:35Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

dkarrasch added the linear algebra Linear algebra label Jul 19, 2021

andreasnoack reviewed Jul 20, 2021

View reviewed changes

dkarrasch commented Jul 23, 2021

View reviewed changes

simeonschaub mentioned this pull request Jul 24, 2021

upcoming changes to cholesky in base JuliaDiff/ChainRules.jl#477

Closed

dkarrasch mentioned this pull request Dec 9, 2021

compat fixes for Julia v1.8 JuliaStats/GLM.jl#459

Merged

dkarrasch added 8 commits December 9, 2021 16:09

Replace Val-types in cholesky[!]

f4eabf0

fix test

63f3279

correct deprecation

40a66bb

add NEWS entry

4d39fac

don't export old methods

38c616d

deprecate each method individually to avoid ambiguity

5f9a1e7

include Diagonal

9a39f38

fix factorization test

d6eba05

dkarrasch force-pushed the dk/cholesky branch from 050dec8 to d6eba05 Compare December 9, 2021 15:16

dkarrasch mentioned this pull request Dec 9, 2021

compat changes for Julia v1.8 fkastner/NiceNumbers.jl#6

Merged

dkarrasch mentioned this pull request Dec 13, 2021

Remove pivot argument w/ default value in cholesky! joshday/SparseRegression.jl#36

Merged

dkarrasch merged commit 490d78c into master Dec 14, 2021

dkarrasch deleted the dk/cholesky branch December 14, 2021 14:55

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022

Replace Val-types in cholesky[!] (JuliaLang#41640)

e166666

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022

Replace Val-types in cholesky[!] (JuliaLang#41640)

58973de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Val-types in cholesky[!] #41640

Replace Val-types in cholesky[!] #41640

dkarrasch commented Jul 19, 2021 •

edited

Loading

andreasnoack Jul 20, 2021

dkarrasch Jul 20, 2021

dkarrasch Jul 20, 2021

dkarrasch Jul 20, 2021

andreasnoack Jul 20, 2021

dkarrasch Jul 20, 2021

andreasnoack Jul 20, 2021

thchr commented Jul 20, 2021

dkarrasch commented Jul 21, 2021

nanosoldier commented Jul 21, 2021

dkarrasch commented Jul 21, 2021

dkarrasch commented Jul 21, 2021

nanosoldier commented Jul 21, 2021

dkarrasch commented Jul 22, 2021

nanosoldier commented Jul 22, 2021

dkarrasch commented Jul 23, 2021

dkarrasch Jul 23, 2021

palday commented Aug 4, 2021 •

edited

Loading

dkarrasch commented Aug 6, 2021

palday commented Aug 6, 2021

dkarrasch commented Dec 9, 2021

nanosoldier commented Dec 9, 2021

palday commented Dec 9, 2021

dkarrasch commented Dec 13, 2021

dkarrasch commented Dec 13, 2021

palday commented Dec 13, 2021

dkarrasch commented Dec 14, 2021

nanosoldier commented Dec 14, 2021

	cpapd = cholesky(apdh, RowMaximum())
	cpapd = cholesky(apdh, RowMaximum())

		positive semi-definite matrix `A`. This is the return type of [`cholesky(_, Val(true))`](@ref),
		positive semi-definite matrix `A`. This is the return type of [`cholesky(_, ::RowMaximum)`](@ref),

Replace Val-types in cholesky[!] #41640

Replace Val-types in cholesky[!] #41640

Conversation

dkarrasch commented Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thchr commented Jul 20, 2021

dkarrasch commented Jul 21, 2021

nanosoldier commented Jul 21, 2021

dkarrasch commented Jul 21, 2021

dkarrasch commented Jul 21, 2021

nanosoldier commented Jul 21, 2021

dkarrasch commented Jul 22, 2021

nanosoldier commented Jul 22, 2021

dkarrasch commented Jul 23, 2021

Choose a reason for hiding this comment

palday commented Aug 4, 2021 • edited Loading

dkarrasch commented Aug 6, 2021

palday commented Aug 6, 2021

dkarrasch commented Dec 9, 2021

nanosoldier commented Dec 9, 2021

palday commented Dec 9, 2021

dkarrasch commented Dec 13, 2021

dkarrasch commented Dec 13, 2021

palday commented Dec 13, 2021

dkarrasch commented Dec 14, 2021

nanosoldier commented Dec 14, 2021

dkarrasch commented Jul 19, 2021 •

edited

Loading

palday commented Aug 4, 2021 •

edited

Loading