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

Fix order of triangular and adjortrans wrappers #283

Merged
merged 4 commits into from
Nov 2, 2022

Conversation

dkarrasch
Copy link
Member

I'm not sure why this hasn't come up anywhere, but it may be that it simply lead to performance penalties. In any case, for a while adjoints of triangular matrices are passed to the wrapped matrix while the triangular wrapper is flipped. This should be backported to v1.8.

@dkarrasch
Copy link
Member Author

Hm, the swap already happened in the v1.6 cycle: JuliaLang/julia#38168, so we need to backport this.

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2022

Codecov Report

Merging #283 (aa7d4b8) into main (fa54768) will increase coverage by 1.74%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #283      +/-   ##
==========================================
+ Coverage   92.03%   93.78%   +1.74%     
==========================================
  Files          12       12              
  Lines        7334     7334              
==========================================
+ Hits         6750     6878     +128     
+ Misses        584      456     -128     
Impacted Files Coverage Δ
src/linalg.jl 95.50% <100.00%> (+11.07%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rayegun
Copy link
Member

rayegun commented Oct 30, 2022

So we need to queue this up for backports to 1.6, 1.7 and 1.8?

@dkarrasch
Copy link
Member Author

dkarrasch commented Oct 30, 2022

How embarrassing, and the fault is all mine:

julia> @which lmul!(UpperTriangular(sprand(10,10,0.3))', zeros(10,4))
lmul!(xA::LowerTriangular{var"#s804", var"#s803"} where {var"#s804", var"#s803"<:Adjoint}, B::StridedVecOrMat{T} where T) in LinearAlgebra at /Applications/Julia-1.6.app/Contents/Resources/julia/share/julia/stdlib/v1.6/LinearAlgebra/src/triangular.jl:1013

So we need to queue this up for backports to 1.6, 1.7 and 1.8?

Yes, for 1.6 and 1.8.

@dkarrasch dkarrasch merged commit bdd0197 into main Nov 2, 2022
@dkarrasch dkarrasch deleted the dk/adjortranstriangle branch November 2, 2022 09:31
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

The backport to 1.8 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.8 1.8
# Navigate to the new working tree
cd .worktrees/backport-1.8
# Create a new branch
git switch --create backport-283-to-1.8
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 bdd01975cbb3c11a1042eb247baa88cec1ada5de
# Push it to GitHub
git push --set-upstream origin backport-283-to-1.8
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.8

Then, create a pull request where the base branch is 1.8 and the compare/head branch is backport-283-to-1.8.

vtjnash added a commit to JuliaLang/julia that referenced this pull request Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants