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.

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

force sym/herm back to return Matrix (1.6 inference fix) #393

Merged
merged 2 commits into from
Mar 30, 2021
Merged

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Mar 29, 2021

Fixed the type inference failures we have been seeing in 1.6.
They show up because:

On 1.5:

julia> L = LowerTriangular(rand(3,3));

julia> U = UpperTriangular(rand(3,3));

julia> typeof(L .+ U')
Array{Float64,2}

On 1.6:

julia> L = LowerTriangular(rand(3,3));

julia> U = UpperTriangular(rand(3,3));

julia> typeof(L .+ U')
LowerTriangular{Float64, Matrix{Float64}}

Which honestly is kinda cool, but not what we want if we are to be type stable
Possibly we should push uplo into the type domain with a value type,
but that is beyond the scope of this PR.
The other branchs force the result to be a Matrix so the last should too

@oxinabox oxinabox requested a review from sethaxen March 29, 2021 18:17
@github-actions github-actions bot added the needs version bump Version needs to be incremented or set to -DEV in Project.toml label Mar 29, 2021
@oxinabox oxinabox changed the title force sym/herm back to return Matrix force sym/herm back to return Matrix (1.6 inference fix) Mar 29, 2021
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Patch bump please :)

Copy link
Member

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

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

Thanks for solving this

@github-actions github-actions bot removed the needs version bump Version needs to be incremented or set to -DEV in Project.toml label Mar 29, 2021
@sethaxen
Copy link
Member

Perhaps instead of converting to Matrix, it would be better to explicitly call parent if the produced cotangent is a LowerTriangular and then use tril!?

@oxinabox
Copy link
Member Author

Perhaps instead of converting to Matrix, it would be better to explicitly call parent if the produced cotangent is a LowerTriangular and then use tril!?

We call Matrix in ther over branchs of this.

If there is a more efficient way should it be PRed to Base for convert ? (convert right now falls back to Matrix unless result is already a Matrix)
Possibly this code in all branchs should be rewritten to use convert to avoid copying in the case it is already a Matrix

@sethaxen
Copy link
Member

We call Matrix in ther over branchs of this.

Oh you're right, then yes, this is fine. But a future PR should fix that.

Possibly this code in all branchs should be rewritten to use convert to avoid copying in the case it is already a Matrix

My suggestion is to avoid converting to Matrix at all. Certain non-Matrix types have good support for being wrapped with Symmetric, e.g. StaticMatrix. Maybe sparse matrices. So in the spirit of the discussion in #232 it would be good to avoid making the cotangent a Matrix. The idea would be to let broadcast sort it out but handle this potential type-instability by calling parent on an AbstractTriangular matrix. But this can be a future PR.

If there is a more efficient way should it be PRed to Base for convert ? (convert right now falls back to Matrix unless result is already a Matrix)

A more efficient way to do what?

@codecov-io
Copy link

codecov-io commented Mar 29, 2021

Codecov Report

Merging #393 (cf106e7) into master (67c106f) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
- Coverage   98.49%   98.43%   -0.06%     
==========================================
  Files          22       22              
  Lines        1855     1857       +2     
==========================================
+ Hits         1827     1828       +1     
- Misses         28       29       +1     
Impacted Files Coverage Δ
src/rulesets/LinearAlgebra/symmetric.jl 99.61% <100.00%> (ø)
src/rulesets/Base/fastmath_able.jl 98.24% <0.00%> (-0.88%) ⬇️
src/rulesets/LinearAlgebra/norm.jl 100.00% <0.00%> (ø)
src/rulesets/LinearAlgebra/factorization.jl 97.83% <0.00%> (+0.01%) ⬆️
src/rulesets/Base/evalpoly.jl 97.82% <0.00%> (+0.02%) ⬆️

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 67c106f...cf106e7. Read the comment docs.

@oxinabox oxinabox merged commit 4e3164a into master Mar 30, 2021
@oxinabox oxinabox deleted the ox/infer branch March 30, 2021 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants