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

Add left division operator #82

Merged
merged 10 commits into from
Nov 24, 2021
Merged

Add left division operator #82

merged 10 commits into from
Nov 24, 2021

Conversation

theogf
Copy link
Contributor

@theogf theogf commented Nov 23, 2021

This follows from the discussion from #39. This is the same implementation presented in the issue.
It is currently done with in place allocation, but I could also change it if you think it would be better.

@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #82 (7127e3f) into master (368887a) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage   95.65%   95.85%   +0.20%     
==========================================
  Files           5        5              
  Lines         322      314       -8     
==========================================
- Hits          308      301       -7     
+ Misses         14       13       -1     
Impacted Files Coverage Δ
src/linalg.jl 95.18% <100.00%> (+0.44%) ⬆️
src/base_maths.jl 100.00% <0.00%> (ø)
src/chainrules.jl 100.00% <0.00%> (ø)
src/blockdiagonal.jl 85.71% <0.00%> (+0.63%) ⬆️

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 368887a...7127e3f. Read the comment docs.

@theogf
Copy link
Contributor Author

theogf commented Nov 23, 2021

@dkarrasch in #39 (comment) what did you mean by checking that the blocks are diagonal?

src/linalg.jl Outdated Show resolved Hide resolved
theogf and others added 2 commits November 24, 2021 12:20
Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
src/linalg.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Maybe let's just add a test for nonsquare matrices and we are good to go :)

Oh, and CI and version bump!

theogf and others added 3 commits November 24, 2021 13:01
Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
@mzgubic
Copy link
Collaborator

mzgubic commented Nov 24, 2021

Hmm, what's up with CI?

@theogf
Copy link
Contributor Author

theogf commented Nov 24, 2021

The new test I added, has issues visibly 😅

@theogf
Copy link
Contributor Author

theogf commented Nov 24, 2021

Oh that's because I am using B which somehow turns into a BlockDiagonal in the other tests.
Which I guess is an issue in itself
[EDIT:] ok so more precisely the Cholesky and SVD tests build their own B which replaces the one defined at the beginning. That's a weird scoping behavior though.
I could give new variable names in the Cholesky and SVD tests to solve it

Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

Thanks! One main question about the error case here

src/linalg.jl Outdated Show resolved Hide resolved
result = similar(vm)
for block in blocks(B)
nrow = size(block, 1)
result[row_i:(row_i + nrow - 1), :] = block \ vm[row_i:(row_i + nrow - 1), :]
Copy link
Contributor

Choose a reason for hiding this comment

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

i've not thought too much, but perhaps you know: is there any rewriting of this that would allow us to access vm col-wise rather than row-wise?

also does using views of vm help performance here at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think accessing vm colwise is possible for this problem.
views could help I suppose. I am going to run some benchmark and see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Views does not seem to help that much in this simple example

julia> using BenchmarkTools
julia> N = 400
julia> A = BlockDiagonal([rand(N, N) for _ in 1:5])
julia> x = rand(5N, 100)
julia> A \ x
julia> @btime $A \ $x # No views
  7.085 ms (37 allocations: 10.70 MiB)
julia> @btime $A \ $x # With views
  7.403 ms (27 allocations: 9.17 MiB)

Copy link
Contributor

@nickrobinson251 nickrobinson251 Nov 24, 2021

Choose a reason for hiding this comment

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

i think the reduced allocations suggests let's use views:

e.g. on a matrix of the same size, but more blocks:

julia> M = 40
julia> B = BlockDiagonal([rand(M, M) for _ in 1:5])
julia> x = rand(5N, 100)
julia> @btime $A \ $x # No views
  2.155 ms (302 allocations: 5.22 MiB)
julia> @btime $A \ $x # With views
  2.264 ms (202 allocations: 3.69 MiB)

(i know the times are ~equal but i usually trust allocations as at least as good a guide in practice)

test/linalg.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

thanks again!

@mzgubic mzgubic merged commit e0ea72c into JuliaArrays:master Nov 24, 2021
@theogf theogf deleted the left_div branch November 24, 2021 15:01
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.

3 participants