Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix lu deprecation warnings on Julia nightly #434

Merged
merged 4 commits into from
Jun 8, 2021
Merged

fix lu deprecation warnings on Julia nightly #434

merged 4 commits into from
Jun 8, 2021

Conversation

simeonschaub
Copy link
Member

@oxinabox
Copy link
Member

oxinabox commented Jun 2, 2021

Should we put these in Compat.jl instead?

@simeonschaub
Copy link
Member Author

I thought about that, but wasn't sure defining Compat.RowMaximum and Compat.NoPivot as just Val{true}/Val{false} was the right thing to do for Compat.jl, or whether it should define them as separate singletons and overload lu.

@oxinabox
Copy link
Member

oxinabox commented Jun 2, 2021

I don't think we are in any rush to fix this.
So lets open a PR on Compat.jl and see what the Compat.jl maintainers think?

@simeonschaub
Copy link
Member Author

Ah, looking at the Julia PR again it seems like Val{true} only means RowMaximum for lu, while for qr Val{true} means ColumnNorm. This likely means we don't want to implement them as aliases for Val{true} in Compat, because lu(x, ColumnNorm()) would return lu(x, RowMaximum()) without any warning, which seems bad. I can open a PR defining these as singletons in Compat instead, but that would be independent of this PR, since we do actually need to overload lu(x, ::Val) on earlier Julia versions.

@oxinabox
Copy link
Member

oxinabox commented Jun 6, 2021

I can open a PR defining these as singletons in Compat instead, but that would be independent of this PR, since we do actually need to overload lu(x, ::Val) on earlier Julia versions.

let's do this

oxinabox
oxinabox reviewed Jun 6, 2021
View reviewed changes
src/rulesets/LinearAlgebra/factorization.jl Outdated
@@ -15,11 +15,14 @@ using LinearAlgebra.BLAS: gemv, gemv!, gemm!, trsm!, axpy!, ger!
# for derivations for wide and tall matrices, see
# https://sethaxen.com/blog/2021/02/differentiating-the-lu-decomposition/

const _RowMaximum = VERSION >= v"1.7.0-DEV.1188" ? RowMaximum : Val{true}
const _NoPivot = VERSION >= v"1.7.0-DEV.1188" ? NoPivot : Val{false}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to call this LU_NoPivot
since we will want this to differ for QR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, makes sense

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2021 •
edited
Loading

Codecov Report

Merging #434 (b4a36bf) into master (a1809f4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #434   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files          21       21           
  Lines        1989     1989           
=======================================
  Hits         1957     1957           
  Misses         32       32
Impacted Files Coverage Δ
src/rulesets/LinearAlgebra/factorization.jl 97.85% <100.00%> (ø)

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 a1809f4...b4a36bf. Read the comment docs.

@simeonschaub simeonschaub requested a review from oxinabox June 8, 2021 10:21
oxinabox
oxinabox approved these changes Jun 8, 2021
View reviewed changes
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

merge when you are happy

@simeonschaub @oxinabox
Apply suggestions from code review
547b462 
Co-authored-by: Lyndon White <oxinabox@ucc.asn.au>
simeonschaub added a commit that referenced this pull request Jun 8, 2021
@simeonschaub
use @maybe_inferred instead of @inferred
1363239 
ref JuliaDiff/ChainRulesTestUtils.jl#165.

I will bump the version as well once #434 is merged so that PkgEval
picks this up.
@simeonschaub simeonschaub mentioned this pull request Jun 8, 2021
Merged
@simeonschaub simeonschaub merged commit 407d332 into master Jun 8, 2021
@simeonschaub simeonschaub deleted the sds/fix_lu_depr branch June 8, 2021 15:58
simeonschaub added a commit that referenced this pull request Jun 8, 2021
@simeonschaub @oxinabox
fix lu deprecation warnings on Julia nightly (#434)
902b6a0 
Ref JuliaLang/julia#40623

Co-authored-by: Lyndon White <oxinabox@ucc.asn.au>
simeonschaub added a commit that referenced this pull request Jun 8, 2021
@simeonschaub
use @maybe_inferred instead of @inferred (#438)
5c46ba5 
ref JuliaDiff/ChainRulesTestUtils.jl#165.

I will bump the version as well once #434 is merged so that PkgEval
picks this up.
@simeonschaub simeonschaub mentioned this pull request Jul 24, 2021
Closed
@sethaxen sethaxen mentioned this pull request Jun 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxinabox oxinabox oxinabox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simeonschaub @oxinabox @codecov-commenter