un-revert "Simplify some views of Adjoint matrices" #1122
Conversation
|
The windows CI error happens spuriously and even happens on Julia repo tests sometimes. |
mcabbott
force-pushed
the
adjointview
branch
from
e71fdab to
433a3ba
Compare
|
Bump? After rebasing this passes CI. The original PR was approved, but reverted because it broke the tests of some later PR (which was merged first). That code has since been edited not to rely on the absence of this optimisation, which is why tests pass again now. (In fact I have a merge button, but am not sure what the rules are on my using it, and thus would prefer someone else to approve.) |
|
|
||
| # these make eachrow(A') produce simpler views | ||
| @inline Base.unsafe_view(A::Transpose{<:Number, <:AbstractMatrix}, i::Integer, j::AbstractArray) = | ||
| Base.unsafe_view(parent(A), j, i) |
There was a problem hiding this comment.
These should probably call view on the parent, as the parent might have a specialized view implementation that avoids the SubArray wrapper.
There was a problem hiding this comment.
Can't we also do something similar for transposes of AbstractVectors, with one AbstractVector index?
There was a problem hiding this comment.
I initially said the wrong thing, because I've forgotten how this all works, but in fact that case is already handled (and tested). It's why the reshape(v, ::Val{1}) method is defined above.
I'm hesitant to start editing as I do not have the details fresh in my head, and do not wish to introduce fresh mistakes. The pervious PR reverted mine with no changes, this PR un-does the reversion.
| Base.reshape(v::AdjointAbsVec{<:Real}, ::Val{1}) = parent(v) | ||
|
|
||
| # these make eachrow(A') produce simpler views | ||
| @inline Base.unsafe_view(A::Transpose{<:Number, <:AbstractMatrix}, i::Integer, j::AbstractArray) = |
There was a problem hiding this comment.
Perhaps use the alias TransposeAbsMat like the TransposeAbsVec alias used in reshape?
There was a problem hiding this comment.
IDK why it's written this way, it's possible I followed nearby code which writes e.g. Base.strides(A::Adjoint{<:Real, <:AbstractMatrix}) and does not seem to use TransposeAbsMat. Maybe I was concerned about order of definitions or something?
| vec(v::TransposeAbsVec{<:Number}) = parent(v) | ||
| vec(v::AdjointAbsVec{<:Real}) = parent(v) | ||
|
|
||
| Base.reshape(v::TransposeAbsVec{<:Number}, ::Val{1}) = parent(v) |
There was a problem hiding this comment.
Should reshape(v, ::Colon) and such behave identically? Or is the Val(1) reshaping used in the view below? E.g.:
julia> v = 1:4
1:4
julia> reshape(v', :)
4-element reshape(adjoint(::UnitRange{Int64}), 4) with eltype Int64:
1
2
3
4
julia> reshape(v', Val(1))
1:4It might be a good idea to have these all return a range, but that may be a separate PR.
There was a problem hiding this comment.
Yea this PR is only about view. And some views call this (internal?) method with Val.
The "supported" way to do that reshape is:
julia> vec(v')
1:4The default definition is vec(a::AbstractArray) = reshape(a,length(a)), and at present reshape(v', 4) also has no shortcut.
3 participants
This copies JuliaLang/julia#43719 to the new LinearAlgebra.jl repository (partly to see if this all works!) The original PR was JuliaLang/julia#39467
Tests passed on CI in Feb, but I have not checked anything locally.
(Closes JuliaLang/julia#43719)