Skip to content

Use new cholesky pivot syntax in v1.8#633

Merged
sethaxen merged 5 commits intomainfrom
choleskypivot
Aug 4, 2022
Merged

Use new cholesky pivot syntax in v1.8#633
sethaxen merged 5 commits intomainfrom
choleskypivot

Conversation

@sethaxen
Copy link
Member

Fixes #477 by adopting a similar approach as #434, except that it uses a type union to still support the old syntax. This will need to be removed for future Julia versions when the deprecated syntax is removed.

@github-actions github-actions bot added the needs version bump Version needs to be incremented or set to -DEV in Project.toml label Jun 17, 2022
@sethaxen sethaxen requested a review from simeonschaub June 17, 2022 21:02
@github-actions github-actions bot removed the needs version bump Version needs to be incremented or set to -DEV in Project.toml label Jun 17, 2022
@github-actions github-actions bot added the needs version bump Version needs to be incremented or set to -DEV in Project.toml label Jun 23, 2022
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Mainly a version bump seems to be missing?

I think this would fix some downstream issues in Zygote on newer Julia versions. These issues popped up also in downstream packages such as JuliaGaussianProcesses/ApproximateGPs.jl#135. I assume this PR might fix them since one issue seems to be that currently the rules for cholesky in ChainRules are not picked up (see e.g. https://github.com/FluxML/Zygote.jl/runs/7124557589?check_suite_focus=true#step:6:1540).

@oschulz
Copy link

oschulz commented Jul 15, 2022

Closes #647

@oschulz
Copy link

oschulz commented Jul 17, 2022

Should also fix current Zygote failure on cholesky(::PDiagMat) in Julia v1.8.

@github-actions github-actions bot added needs version bump Version needs to be incremented or set to -DEV in Project.toml and removed needs version bump Version needs to be incremented or set to -DEV in Project.toml labels Aug 4, 2022
@sethaxen sethaxen merged commit 1f4a8a9 into main Aug 4, 2022
@sethaxen sethaxen deleted the choleskypivot branch August 4, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs version bump Version needs to be incremented or set to -DEV in Project.toml

Projects

None yet

Development

Successfully merging this pull request may close these issues.

upcoming changes to cholesky in base

3 participants