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 one and copy for Diagonal #52

Merged
merged 7 commits into from
Mar 7, 2021
Merged

Conversation

putianyi889
Copy link
Contributor

@putianyi889 putianyi889 commented Nov 20, 2020

@putianyi889 putianyi889 changed the title Update infarrays.jl Add one and copy for Diagonal Nov 20, 2020
src/infarrays.jl Outdated
@@ -202,6 +202,8 @@ end
# Diagonal
#####

one(D::Diagonal{T,<:AbstractFill}) where T = Eye{T}(size(D,1))
Copy link
Member

Choose a reason for hiding this comment

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

This counts as "type piracy": it changes behaviour of FillArrays.jl which is bad.

Instead restrict to the infinite case:

one(D::Diagonal{T,<:AbstractFill{T,Tuple{OneToInf{Int}}}) where T = Eye{T}(size(D,1))
copy(D::Diagonal{T,<:AbstractFill{T,Tuple{OneToInf{Int}}}) where T = D

Though this could be refined to allow for infinite offset indexing.

test/runtests.jl Outdated
@@ -627,6 +627,7 @@ end

@test Eye{Int}(∞) * D ≡ Eye{Int}(∞) * D ≡ D
@test Eye(∞) * D == Eye(∞) * D == D
@test Eye(∞) == Eye(∞)^0 == Eye(∞)^1
Copy link
Member

Choose a reason for hiding this comment

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

Add tests for one and copy directly as well.

Copy link
Member

Choose a reason for hiding this comment

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

Add tests that Eye(∞) == Eye(∞)^2

Copy link
Member

Choose a reason for hiding this comment

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

Add tests for Diagonal(Fill(2,∞))

Copy link
Contributor Author

@putianyi889 putianyi889 Nov 20, 2020

Choose a reason for hiding this comment

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

^2 shouldn't be a problem because LinearAlgebra deals with matrix powers like this:

# base\intfuncs.jl:243
function power_by_squaring(x_, p::Integer)
    x = to_power_type(x_)
    if p == 1
        return copy(x)
    elseif p == 0
        return one(x)
    elseif p == 2
        return x*x
    elseif p < 0
        isone(x) && return copy(x)
        isone(-x) && return iseven(p) ? one(x) : copy(x)
        throw_domerr_powbysq(x, p)
    end
    t = trailing_zeros(p) + 1
    p >>= t
    while (t -= 1) > 0
        x *= x
    end
    y = x
    while p > 0
        t = trailing_zeros(p) + 1
        p >>= t
        while (t -= 1) >= 0
            x *= x
        end
        y *= x
    end
    return y
end

I added the test anyway.

@putianyi889
Copy link
Contributor Author

putianyi889 commented Nov 20, 2020

Actually I encountered this problem when doing M^0 where M is infinite and banded. Shall we fix this problem in a more general way?

Tridiagonal has the same issues.

@dlfivefifty
Copy link
Member

dlfivefifty commented Nov 20, 2020

The right place to do this is in ArrayLayouts.jl. It's not possible to do this for every array but we can do it for LayoutArray, which is the super type for BandedMatrix, BlockArray, etc. and enables adding features to arrays that aren't supported for base. So we could add to ArrayLayouts.jl something like

_one(_, _, d::AbstractMatrix{T}) where T = Base.invoke(one, Tuple{AbstractMatrix{T}}, d) # standard def
function _one(::DiagonalLayout, _, d::AbstractMatrix{T}) where T
    m,n = size(d)
     m==n || throw(DimensionMismatch("multiplicative identity defined only for square matrices"))
    Eye{T}(m)
end
# ... Tridiagonal, etc., special cases
one(d::LayoutMatrix) = _one(MemoryLayout(d), axes(d), d)
one(d::SubArray{<:Any,2,<:LayoutMatrix}) = _one(MemoryLayout(d), axes(d), d)
one(d::Diagonal{<:Any,<:LayoutMatrix}) = _one(MemoryLayout(d), axes(d), d)
one(d::Tridiagonal{<:Any,<:LayoutVector}) = _one(MemoryLayout(d), axes(d), d)
one(d::SymTridiagonal{<:Any,<:LayoutVector}) = _one(MemoryLayout(d), axes(d), d)

Then in BandedMatrices.jl:

function _one(::AbstractBandedLayout, _, d::AbstractMatrix{T}) where T
    m,n = size(d)
     m==n || throw(DimensionMismatch("multiplicative identity defined only for square matrices"))
    convert(BandedMatrix, Eye{T}(m))
end

@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #52 (2dddc9d) into master (a9437da) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   77.44%   77.51%   +0.07%     
==========================================
  Files           4        4              
  Lines         625      627       +2     
==========================================
+ Hits          484      486       +2     
  Misses        141      141              
Impacted Files Coverage Δ
src/infarrays.jl 76.28% <100.00%> (+0.30%) ⬆️

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 a9437da...2dddc9d. Read the comment docs.

@dlfivefifty dlfivefifty merged commit 058032d into JuliaArrays:master Mar 7, 2021
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.

2 participants