Use to_indices in the OffsetArray constructor #157

jishnub · 2020-09-21T12:47:28Z

This PR relaxes the types that may be passed to the constructor. After this, any type that may be converted to an AbstractUnitRange may be passed, including types defined in an external package.

For example, using EllipsisNotation.jl,

julia> OffsetArray(zeros(2,2,1), .., 3:3)
2×2×1 OffsetArray(::Array{Float64,3}, 1:2, 1:2, 3:3) with eltype Float64 with indices 1:2×1:2×3:3:
[:, :, 3] =
 0.0  0.0
 0.0  0.0

the leading axes are replaced by colons, and only the last index is offset.

The PR also fixes some indexing issues with IdOffsetRanges. Now indexing with offset rages are also idempotent.

julia> r = OffsetArrays.IdOffsetRange(3:5, -1)
OffsetArrays.IdOffsetRange(2:4)

julia> s = OffsetArrays.IdOffsetRange(-3:-2, 4)
OffsetArrays.IdOffsetRange(1:2)

julia> axes(r)
(OffsetArrays.IdOffsetRange(0:2),)

julia> r[s]
OffsetArrays.IdOffsetRange(3:4)

julia> axes(r[s]) == axes(s)
true

Also define Base.firstindex for an IdOfffsetRange to make it marginally faster wrt. master, now it's as fast as the parent.

julia> @btime firstindex($(Ref(r))[].parent)
  2.670 ns (0 allocations: 0 bytes)
1

julia> @btime firstindex($(Ref(r))[])
  2.670 ns (0 allocations: 0 bytes)
0

vs

# on master
julia> @btime firstindex($(Ref(r))[].parent)
  2.670 ns (0 allocations: 0 bytes)
1

julia> @btime firstindex($(Ref(r))[])
  3.196 ns (0 allocations: 0 bytes)
0

codecov · 2020-09-21T13:01:47Z

Codecov Report

Merging #157 into master will increase coverage by 5.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
+ Coverage   94.60%   99.60%   +5.00%     
==========================================
  Files           4        4              
  Lines         241      256      +15     
==========================================
+ Hits          228      255      +27     
+ Misses         13        1      -12

Impacted Files	Coverage Δ
src/OffsetArrays.jl	`99.44% <100.00%> (+4.01%)`	⬆️
src/axes.jl	`100.00% <100.00%> (+9.09%)`	⬆️
src/utils.jl	`100.00% <100.00%> (+6.25%)`	⬆️

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 2ed1598...e3f8fd9. Read the comment docs.

johnnychen94 · 2020-09-21T13:30:40Z

Haven't taken a look at the details but it looks nice. Should I get #148 in first, or get this in first?

Either way it requires some extra rebasing work to solve the conflicts.

jishnub · 2020-09-21T13:49:07Z

Maybe #148 can go first as it is less of a change, more of a cleanup

jishnub · 2020-09-24T07:35:20Z

I think it's worth splitting this PR into two, with the bugfixes and performance improvements going into the 1.2 series and the added feature into 1.3

johnnychen94 · 2020-09-24T07:41:46Z

Oh. I haven’t thought about branching developments. I was thinking about squashing all these commits into 1.3.0 release. The Origin features should be released in 1.3.0; this kind of blocks your plan unless we revert it :sad: Johnny Chen 陈久宁 School of Mathematical Sciences, East China Normal University, Shanghai 华东师范大学数学科学学院 2020年9月24日 +0800 15:35 Jishnu Bhattacharya <notifications@github.com>，写道：

…

I think it's worth splitting this PR into two, with the bugfixes and performance improvements going into the 1.2 series and the added feature into 1.3 — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

jishnub · 2020-09-24T08:40:17Z

As per semver, bugfixes should go in patch releases and features should go in minor releases. Origin and using to_indices therefore belong to 1.3. However bugfixes should go in 1.2.x.

Maybe once Origin is merged I can rebase on top of that if we target 1.3, otherwise it's for 1.4.

johnnychen94 · 2020-09-24T08:50:36Z

As per semver, bugfixes should go in patch releases and features should go in minor releases.

Strictly speaking, yes. I see the constructor rework and the origin feature as new features, so I'm targeting 1.3.0 and no more 1.2.x release. That said, I think it is totally fine to squash some bug fixes into a minor release; this frees us from managing different release branches with cherry-picks and back-ports or so...

Maybe once Origin is merged I can rebase on top of that if we target 1.3, otherwise it's for 1.4.

LOL #147 is already merged.

jishnub · 2020-09-25T21:35:57Z

This PR also modifies this behavior on master:

# on master
julia> OffsetVector(zeros(2,2), 1:2, 1:2)
2×2 OffsetArray(::Array{Float64,2}, 1:2, 1:2) with eltype Float64 with indices 1:2×1:2:
 0.0  0.0
 0.0  0.0

# this PR
julia> OffsetVector(zeros(2,2), 1:2, 1:2)
ERROR: ArgumentError: OffsetVector requires a 1D array

johnnychen94

Except for the additional dispatch complexity and the tendency to somehow revert #148 🤣 , it looks really nice!

src/OffsetArrays.jl

src/utils.jl

src/OffsetArrays.jl

src/utils.jl

src/OffsetArrays.jl

docs/src/internals.md

to_indices converts colons to ranges. This is followed by converting CartesianIndices to ranges. Fix indexing IdOffsetRange using offset ranges, ensuring that indexing is idempotent

src/OffsetArrays.jl

johnnychen94

Thanks for doing this, really nice an improvement to make this generic! The test looks really awesome, too.

src/utils.jl

src/OffsetArrays.jl

jishnub · 2020-09-27T05:27:06Z

src/OffsetArrays.jl

    # In general, indices get converted to AbstractUnitRanges.
    # CartesianIndices{N} get converted to N ranges
-    @eval function $FT(A::AbstractArray, inds::Tuple)
+    @eval function $FT(A::AbstractArray, inds::Tuple{Any,Vararg{Any}})


This isn't really necessary given the dispatch priority of Tuple{Vararg{Integer}} over Tuple, but helps to clarify that this method doesn't handle empty tuples.

jishnub · 2020-09-27T05:32:45Z

src/OffsetArrays.jl

+    # Nested OffsetArrays may strip off the wrapper and collate the offsets
+    @eval function $FT(A::OffsetArray, offsets::Tuple{Vararg{Integer}})
+        _checkindices(A, offsets, "offsets")
+        $FT(parent(A), map(+, A.offsets, offsets))


Broadcasting treats values in Tuples as scalars as opposed to treating the Tuple as a collection in itself, so something like (1,2) .+ (1,) evaluates to (2,3). map, on the other hand, operates elementwise. This is not strictly necessary as _checkindices catches it anyway, but might help against future bugs.

johnnychen94 · 2020-09-27T13:01:40Z

@jishnub you're doing really great work! Feel free to merge when you think it is ready.

I think this is the last piece we need to v1.3.0?

jishnub · 2020-09-27T13:32:05Z

Maybe we can include #152 in 1.3 as well, now that julia 1.5.2 has been released

johnnychen94 mentioned this pull request Sep 21, 2020

rework OffsetArrays constructor #148

Merged

jishnub force-pushed the to_indices branch 3 times, most recently from de948d1 to 9f26eb1 Compare September 25, 2020 21:18

johnnychen94 reviewed Sep 26, 2020

View reviewed changes

use to_indices to simplify constructor dispatch

d59a1e2

to_indices converts colons to ranges. This is followed by converting CartesianIndices to ranges. Fix indexing IdOffsetRange using offset ranges, ensuring that indexing is idempotent

jishnub force-pushed the to_indices branch from bb38b54 to d59a1e2 Compare September 26, 2020 11:59

jishnub commented Sep 26, 2020

View reviewed changes

src/OffsetArrays.jl Show resolved Hide resolved

johnnychen94 approved these changes Sep 26, 2020

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/OffsetArrays.jl Show resolved Hide resolved

jishnub added 2 commits September 26, 2020 18:45

comment on error message optimization

29efcd0

simplify constructors

53ba857

jishnub commented Sep 27, 2020

View reviewed changes

remove references to internal functions in docs

2249dc3

Add example with a custom AbstractUnitRange

e3f8fd9

jishnub merged commit b6a4f49 into JuliaArrays:master Sep 27, 2020

This was referenced Nov 15, 2020

Fix OffsetArrays ambiguity resolution again Vexatos/CircularArrays.jl#8

Merged

Type-piracy in similar with OffsetAxis #87

Open

jishnub deleted the to_indices branch January 17, 2021 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use to_indices in the OffsetArray constructor #157

Use to_indices in the OffsetArray constructor #157

jishnub commented Sep 21, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading

johnnychen94 commented Sep 21, 2020 •

edited

Loading

jishnub commented Sep 21, 2020

jishnub commented Sep 24, 2020

johnnychen94 commented Sep 24, 2020 via email

jishnub commented Sep 24, 2020

johnnychen94 commented Sep 24, 2020

jishnub commented Sep 25, 2020

johnnychen94 left a comment

johnnychen94 left a comment

jishnub Sep 27, 2020

jishnub Sep 27, 2020 •

edited

Loading

johnnychen94 commented Sep 27, 2020 •

edited

Loading

jishnub commented Sep 27, 2020

Use to_indices in the OffsetArray constructor #157

Use to_indices in the OffsetArray constructor #157

Conversation

jishnub commented Sep 21, 2020 • edited Loading

codecov bot commented Sep 21, 2020 • edited Loading

Codecov Report

johnnychen94 commented Sep 21, 2020 • edited Loading

jishnub commented Sep 21, 2020

jishnub commented Sep 24, 2020

johnnychen94 commented Sep 24, 2020 via email

jishnub commented Sep 24, 2020

johnnychen94 commented Sep 24, 2020

jishnub commented Sep 25, 2020

johnnychen94 left a comment

Choose a reason for hiding this comment

johnnychen94 left a comment

Choose a reason for hiding this comment

jishnub Sep 27, 2020

Choose a reason for hiding this comment

jishnub Sep 27, 2020 • edited Loading

Choose a reason for hiding this comment

johnnychen94 commented Sep 27, 2020 • edited Loading

jishnub commented Sep 27, 2020

jishnub commented Sep 21, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading

johnnychen94 commented Sep 21, 2020 •

edited

Loading

jishnub Sep 27, 2020 •

edited

Loading

johnnychen94 commented Sep 27, 2020 •

edited

Loading