OffsetArray support for cat/vcat/hcat #37629

johnnychen94 · 2020-09-17T15:36:25Z

When doing concatenation OffsetArrays are treated as plain lists and offsets are ignored.

This support for OffsetArrays only works on OffsetArrays@1 where IdOffsetRange is defined.

Blocked by #37643, the first commit will be removed after that get resolved.

musm · 2020-09-17T16:28:04Z

base/abstractarray.jl

    if dense
-        for k=1:nargs
+        for k=eachindex(A)


spaces around =

mcabbott · 2020-09-18T16:31:32Z

base/abstractarray.jl

@@ -1472,17 +1471,17 @@ function _typed_hcat(::Type{T}, A::AbstractVecOrTuple{AbstractVecOrMat}) where T
        nd = ndims(Aj)
        ncols += (nd==2 ? size(Aj,2) : 1)
    end
-    B = similar(A[1], T, nrows, ncols)
-    pos = 1
+    B = similar(first(A), T, nrows, ncols)


How difficult would it be to adapt this to have B = similar(first(A), T, axes(first(A),1), axes(A,1)), so that all the offsets get propagated?

I guess that's only the case of a vector of vectors; when some elements are matrices you'd have to give up on the 2nd axis.

If I understand your comment correctly.

As explained in JuliaArrays/OffsetArrays.jl#63 (comment), a meaningful offset propagation is only possible for very restricted conditions. This restriction doesn't make cats of OffsetArrays indeed useful.

Oh I didn't see the issue, let me look there first.

timholy

Thanks so much for tackling this! The overall design seems fine.

timholy · 2020-09-19T10:56:39Z

base/abstractarray.jl

-        p1 = pos + Int(length(Vk))::Int - 1
-        a[pos:p1] = Vk
-        pos = p1+1
+        n = length(Vk)


Suggested change

n = length(Vk)

n = Int(length(Vk))::Int

because this often gets called with poor inference. Try @code_warntype Base._typed_vcat(Float64, AbstractVector[[1,2,3], [4.0]]).

Not putting in these annotation re-opens us to all sorts of code invalidation, see https://julialang.org/blog/2020/08/invalidations/.

This isn't a general pattern you need to start adopting in all your Julia code, but this is a very low-level function called by lots of things (all of which would be invalidated if this is invalidated) and from its signature alone you might guess it's a bit of an inference nightmare.

I'm not adding comments like these elsewhere in this function, but you'll probably need to add other annotations to the initialization of pos above. Ideally, it would be great to check with SnoopCompile and loading various packages like Makie to see if you've greatly increased the number of invalidations.

timholy · 2020-09-19T11:03:23Z

base/abstractarray.jl

@@ -1604,7 +1602,7 @@ function __cat(A, shape::NTuple{M,Int}, catdims, X...) where M
    for x in X
        for i = 1:N
            if concat[i]
-                inds[i] = offsets[i] .+ cat_indices(x, i)
+                inds[i] = offsets[i] .+ parent(cat_indices(x, i))


What's this about?

This strips the offsets and uses linear indexing when computing indices. parent(r::IdOffsetRange) = r.parent.

This could be fragile and a little bit tricky, though. Phehaps there's a better way to make things work.

I would generally try to stay away from assumptions about how x might be structured if at all possible...

timholy · 2020-09-19T11:05:34Z

base/abstractarray.jl

-    ncols = size(A[1], 2)
-    for j = 2:nargs
+    ncols = size(first(A), 2)
+    for j = first(axes(A))[2:end]


Suggested change

for j = first(axes(A))[2:end]

for j = firstindex(A)+1:lastindex(A)

timholy · 2020-09-19T11:07:21Z

stdlib/Distributed/test/splitrange.jl

@@ -22,7 +22,7 @@ using Distributed: splitrange
 @test splitrange(-1, 1, 4) == Array{UnitRange{Int64},1}([-1:-1,0:0,1:1])

 const BASE_TEST_PATH = joinpath(Sys.BINDIR, "..", "share", "julia", "test")
-isdefined(Main, :OffsetArrays) || @eval Main include(joinpath($(BASE_TEST_PATH), "testhelpers", "OffsetArrays.jl"))
+isdefined(Main, :OffsetArrays) || @eval Main @everywhere include(joinpath($(BASE_TEST_PATH), "testhelpers", "OffsetArrays.jl"))


What's the reason for this change? Could this cause problems in tests running in other workers? What if it defines methods that are supposed to fail in those tests?

I'm unsure about this, though.

Without this Distributed test is broken:

From worker 10: ERROR: LoadError: LoadError: TaskFailedException From worker 10: From worker 10: nested task error: On worker 25: From worker 10: UndefVarError: OffsetArrays not defined From worker 10: Stacktrace: From worker 10: [1] deserialize_module From worker 10: @ /buildworker/worker/package_linux32/build/usr/share/julia/stdlib/v1.6/Serialization/src/Serialization.jl:957 From worker 10: [2] handle_deserialize From worker 10: @ /buildworker/worker/package_linux32/build/usr/share/julia/stdlib/v1.6/Serialization/src/Serialization.jl:856 From worker 10: [3] deserialize From worker 10: @ /buildworker/worker/package_linux32/build/usr/share/julia/stdlib/v1.6/Serialization/src/Serialization.jl:774

Adding this helps remove this error.

Maybe

julia/test/offsetarray.jl

Line 4 in e378767

using .Main.OffsetArrays

There's already using .Main.OffsetArrays one line down below on this.

I think it's because eachindex now requires OffsetArrays.IdOffsetRange and triggers the error, while for the old version of OffsetArrays it uses IdentityUnitRange and doesn't require anything in OffsetArrays.

It would be better to get #37643 first and I'll drop the OffsetArrays upgrade commit here.

timholy · 2020-09-19T11:08:20Z

test/testhelpers/OffsetArrays.jl


-using Base: Indices, IndexCartesian, IndexLinear, tail
+using Base: Indices, tail, @propagate_inbounds
+@static if !isdefined(Base, :IdentityUnitRange)


This is always defined on master, which is what this PR targets.

I didn't go through the rest carefully, is the idea here to basically "sync" the version here with that in OffsetArrays.jl? Seems reasonable.

Ooops, it turns out that the first c2fd49e commit is broken here and I've separate this commit out in #37643. To first upgrade the OffsetArrays version.

codecov-commenter · 2020-09-19T14:47:29Z

Codecov Report

Merging #37629 into master will increase coverage by 0.04%.
The diff coverage is 84.55%.

@@            Coverage Diff             @@
##           master   #37629      +/-   ##
==========================================
+ Coverage   87.53%   87.57%   +0.04%     
==========================================
  Files         362      362              
  Lines       71830    71888      +58     
==========================================
+ Hits        62873    62953      +80     
+ Misses       8957     8935      -22

Impacted Files	Coverage Δ
base/compiler/ssair/ir.jl	`83.07% <0.00%> (-0.65%)`	⬇️
base/float.jl	`88.26% <ø> (ø)`
base/refpointer.jl	`70.68% <ø> (ø)`
base/refvalue.jl	`100.00% <ø> (ø)`
base/compiler/optimize.jl	`75.00% <50.00%> (ø)`
base/binaryplatforms.jl	`85.38% <82.14%> (+3.53%)`	⬆️
base/compiler/ssair/inlining.jl	`91.61% <85.91%> (-1.46%)`	⬇️
stdlib/Artifacts/src/Artifacts.jl	`61.83% <90.00%> (+8.26%)`	⬆️
base/compiler/ssair/driver.jl	`90.24% <100.00%> (ø)`
base/compiler/typeinfer.jl	`90.23% <100.00%> (ø)`
... and 41 more

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 4ef8313...a61b4e0. Read the comment docs.

jishnub · 2023-02-21T18:36:06Z

Gentle bump, would be good to move this along

musm reviewed Sep 17, 2020

View reviewed changes

base/abstractarray.jl

if dense

for k=1:nargs

for k=eachindex(A)

Copy link

Contributor

musm Sep 17, 2020

Choose a reason for hiding this comment

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

spaces around =

johnnychen94 mentioned this pull request Sep 18, 2020

rework OffsetArrays constructor JuliaArrays/OffsetArrays.jl#148

Merged

johnnychen94 added 2 commits September 18, 2020 16:51

update testhelpers/OffsetArrays

c2fd49e

OffsetArray support for cat/vcat/hcat

a61b4e0

johnnychen94 force-pushed the jc/offset branch from 03bf04f to a61b4e0 Compare September 18, 2020 08:58

This comment has been minimized.

Sign in to view

johnnychen94 mentioned this pull request Sep 18, 2020

upgrade OffsetArrays to v1.3.0 #37643

Merged

mcabbott reviewed Sep 18, 2020

View reviewed changes

timholy reviewed Sep 19, 2020

View reviewed changes

BeastyBlacksmith mentioned this pull request Dec 17, 2021

Fix fillranges with OffsetVectors for plotly JuliaPlots/Plots.jl#4006

Merged

mcabbott mentioned this pull request Aug 27, 2022

Feature request: concatenation may preserve static sizes #46487

Open

brenhinkeller added the arrays [a, r, r, a, y, s] label Nov 20, 2022

jishnub mentioned this pull request Feb 21, 2023

Results of concatenating an offset array JuliaArrays/OffsetArrays.jl#326

Closed

mcabbott mentioned this pull request Mar 12, 2024

vcat / hcat is broken on julia 1.10 mcabbott/AxisKeys.jl#148

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OffsetArray support for cat/vcat/hcat #37629

OffsetArray support for cat/vcat/hcat #37629

johnnychen94 commented Sep 17, 2020 •

edited

Loading

musm Sep 17, 2020

This comment has been minimized.

mcabbott Sep 18, 2020

johnnychen94 Sep 18, 2020 •

edited

Loading

mcabbott Sep 18, 2020

timholy left a comment

timholy Sep 19, 2020

timholy Sep 19, 2020

johnnychen94 Sep 19, 2020

timholy Sep 19, 2020

timholy Sep 19, 2020

timholy Sep 19, 2020

johnnychen94 Sep 19, 2020

timholy Sep 19, 2020

johnnychen94 Sep 19, 2020 •

edited

Loading

timholy Sep 19, 2020

johnnychen94 Sep 19, 2020

codecov-commenter commented Sep 19, 2020

jishnub commented Feb 21, 2023

	for j = first(axes(A))[2:end]
	for j = firstindex(A)+1:lastindex(A)

OffsetArray support for cat/vcat/hcat #37629

Are you sure you want to change the base?

OffsetArray support for cat/vcat/hcat #37629

Conversation

johnnychen94 commented Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

johnnychen94 Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnychen94 Sep 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 19, 2020

Codecov Report

jishnub commented Feb 21, 2023

johnnychen94 commented Sep 17, 2020 •

edited

Loading

johnnychen94 Sep 18, 2020 •

edited

Loading

johnnychen94 Sep 19, 2020 •

edited

Loading