add diag
#86
add diag
#86
Conversation
Pull Request Test Coverage Report for Build 4573381699
💛 - Coveralls |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #86 +/- ##
==========================================
+ Coverage 95.20% 95.25% +0.05%
==========================================
Files 9 9
Lines 938 948 +10
==========================================
+ Hits 893 903 +10
Misses 45 45
☔ View full report in Codecov by Sentry. |
src/ToeplitzMatrices.jl
Outdated
|
|
||
| isconcrete(A::AbstractToeplitz) = isconcretetype(typeof(A.vc)) && isconcretetype(typeof(A.vr)) | ||
| iszero(A::AbstractToeplitz) = iszero(A.vc) && iszero(A.vr) | ||
| diag(A::AbstractToeplitz) = Fill(A.vc[1], min(size(A)...)) |
There was a problem hiding this comment.
This can be done as a fill, right? That way, the dependency on FillArrays isn't needed (although it's not a bad idea in itself)
There was a problem hiding this comment.
As of Julia 1.9, how about using fill as the minimal feature and override it with Fill in the extension?
There was a problem hiding this comment.
We wouldn't want to override a method, that'll hurt precompilation. Extensions are for adding extra methods.
That said, perhaps using FillArrays isn't a bad idea after all
There was a problem hiding this comment.
If both vc and vr are lazy and large then using fill could cause a problem.
range(x, x, length=n) could mimic Fill(x, n) though.
|
drafted to implement |
|
I think the length of the diag needs to be fixed here. julia> T = Toeplitz([0,0,0], [0,4,0])
3×3 Toeplitz{Int64, Vector{Int64}, Vector{Int64}}:
0 4 0
0 0 4
0 0 0
julia> diag(T, 1)
3-element Fill{Int64}, with entries equal to 4
julia> diag(Matrix(T), 1)
2-element Vector{Int64}:
4
4 |
|
also need to test for large |
|
need to test for rectangular ones and negative |
|
Looks good to me |
Pull Request Test Coverage Report for Build 4573311994Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4573381699Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
3 participants
No description provided.