methods for left-division ops involving triangular matrices and sparse vectors #14447
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
More tests are always good. On 1, I think each of the methods could be useful, the challenge is how to implement them in a way that doesn't add up to so much code to maintain. The names of the methods will likely change once we have a transpose type, but the need for implementations of the operation won't suddenly disappear. |
Sacha0
force-pushed
the
trimatldivspvec
branch
from
eeebda6
to
d4844b5
Compare
|
Tests expanded to multiple (three) sparse vectors. Thanks! |
Sacha0
changed the title
|
LGTM. @Sacha0, I assume this one is ready. |
Sacha0
force-pushed
the
trimatldivspvec
branch
from
d4844b5
to
a77a5c9
Compare
|
Rebased and touched up. Should be ready now. Thanks! |
Sacha0
changed the title
Sacha0
force-pushed
the
trimatldivspvec
branch
from
a77a5c9
to
9fe35de
Compare
| @test isapprox((func)(mat, spvec), (func)(mat, fspvec)) | ||
| end | ||
| # test in-place left-division methods not involving quotients | ||
| eltypevec == typeof(zero(eltypemat)*zero(eltypevec) + zero(eltypemat)*zero(eltypevec)) && |
There was a problem hiding this comment.
I'd prefer just if personally, likely better for coverage that way
…s and SparseVectors. Also add tests for those methods.
Sacha0
force-pushed
the
trimatldivspvec
branch
from
9fe35de
to
49c970f
Compare
andreasnoack
added a commit
that referenced
this pull request
Jan 27, 2016
methods for left-division ops involving triangular matrices and sparse vectors
|
Thanks for the review and merge! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses part of JuliaLang/LinearAlgebra.jl#283 by providing methods for left-division operations involving triangular matrices and
SparseVectors. I believe these methods cover all valid operations; please correct me if I missed cases. The implementations are in keeping with the discussion in JuliaLang/LinearAlgebra.jl#283. A few points:(1) I began implementing the corresponding right-division operations via the same approach. But I rapidly found that many relevant underlying methods (among A(t|c)_rdiv_B[t|c][!]) appear to be missing. So I nixed the effort. IIUC (#5332), supporting all such valid methods is not an objective. But should some of those be implemented? If so, which, and does that deserve an issue?
(2) In some cases the in-place operations introduce structural nonzeros that end up numerically zero. As those zeros should only appear in edge cases and stripping them can be expensive, the present implementation does not bother to strip them. Should it? (Edit: Given the discussion in #12605, #9906, and #9928, I guess the answer is no.)
(3) Testing with more than one
SparseVectormight be wise given theSparseVector's nonzero pattern impacts the execution path. Should I add that, or leave the tests as they are?Thanks, and best!