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

Axes for transposed OffsetArray vector incorrect due to LinearAlgebra method #52373

Closed
BioTurboNick opened this issue Dec 2, 2023 · 4 comments · Fixed by #52379
Closed

Axes for transposed OffsetArray vector incorrect due to LinearAlgebra method #52373

BioTurboNick opened this issue Dec 2, 2023 · 4 comments · Fixed by #52379
Labels
domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra

Comments

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Dec 2, 2023

Incorrect axes returned for a transposed OffsetArray vector. This causes incorrect image filtering results for kernels passed as vector components.

See: JuliaImages/ImageFiltering.jl#269

using ImageFiltering
using OffsetArrays

a = centered(zeros(3))'
b = centered(zeros(3)')

axes(a)
# (Base.OneTo(1), OffsetArrays.IdOffsetRange(values=-1:1, indices=-1:1))
# axes(v::Union{LinearAlgebra.Adjoint{T, var"#s972"}, LinearAlgebra.Transpose{T, var"#s972"}} where {T, var"#s972"<:(AbstractVector)}) in LinearAlgebra at src/adjtrans.jl:298

axes(b)
# (OffsetArrays.IdOffsetRange(values=0:0, indices=0:0), OffsetArrays.IdOffsetRange(values=-1:1, indices=-1:1))
# axes(A::OffsetArrays.OffsetArray) in OffsetArrays at src/OffsetArrays.jl:295

(lines based on 1.9 but still present in 1.10)

https://github.com/JuliaLang/julia/blame/8e5136fa2979885081cd502d2210633dff1d2a1a/stdlib/LinearAlgebra/src/adjtrans.jl#L298

@jishnub
Copy link
Contributor

jishnub commented Dec 3, 2023

Looks ok to me? What was the result that you were expecting?

julia> A = zeros(3); axes(A)
(Base.OneTo(3),)

julia> B = centered(A); axes(B) # shift midpoint to zero
(OffsetArrays.IdOffsetRange(values=-1:1, indices=-1:1),)

julia> C = B'; axes(C) # the new axis added by the transpose is not centered about zero
(Base.OneTo(1), OffsetArrays.IdOffsetRange(values=-1:1, indices=-1:1))

julia> D = A'; axes(D)
(Base.OneTo(1), Base.OneTo(3))

julia> E = centered(D); axes(E) # both axes are now centered about zero
(OffsetArrays.IdOffsetRange(values=0:0, indices=0:0), OffsetArrays.IdOffsetRange(values=-1:1, indices=-1:1))

@BioTurboNick
Copy link
Contributor Author

With respect, I think my post is clear in what I expected: The new first dimension should inherit the offset of the parent.

The error in Base though is assuming that the first axis of a transposed vector should always be 1-indexed.

However, I see this is a multi-level issue. Because OffsetArrays assumes all later dimensions start at 1, and would just provide OffsetArrays.IdOffsetRange(values=1:1, indices=1:1) as the axis when asked. I'll take up the matter of that default though with OffsetArrays.

@jishnub
Copy link
Contributor

jishnub commented Dec 3, 2023

I apologize for my brusque tone, but this was not what I had intended. I'll make a PR to change the hard-coded axis to use that of the parent instead.

@BioTurboNick
Copy link
Contributor Author

No worries, and thanks!

@dkarrasch dkarrasch added domain:linear algebra Linear algebra domain:arrays [a, r, r, a, y, s] labels Dec 4, 2023
N5N3 pushed a commit that referenced this issue Dec 5, 2023
Close #52373, or at least the part that may be addressed here. After
this, the first axis for an `Adjoint(parent::AbstractVector)` will be
the second axis of the `parent`. This will change the type of the axis
where the parent array type specializes `axes(A, d)`. In the short term,
this would improve type-stability in cases such as
```julia
julia> A = OffsetArray([1,2], 2);

julia> @code_typed axes(A')[1]
CodeInfo(
1 ─ %1 = $(Expr(:boundscheck))::Bool
│   %2 = Base.getfield(t, i, %1)::OffsetArrays.IdOffsetRange{Int64, Base.OneTo{Int64}}
└──      return %2
) => OffsetArrays.IdOffsetRange{Int64, Base.OneTo{Int64}}
```
where the result is now concretely inferred instead of being a small
`Union`.
In principle, with
JuliaArrays/StaticArrays.jl#916, this would make
the axes of the adjoint of a `StaticVector` statically sized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants