Skip to content

properly handle colon in offset reshape#228

Open
johnnychen94 wants to merge 2 commits intomasterfrom
jc/colon
Open

properly handle colon in offset reshape#228
johnnychen94 wants to merge 2 commits intomasterfrom
jc/colon

Conversation

@johnnychen94
Copy link
Copy Markdown
Member

@johnnychen94 johnnychen94 commented Apr 11, 2021

As a placeholder for a slice, Colon at the k-th position also preserves the offset of the axes(A, k). The default offset for k>=ndims(A) is assumed to be 0.

The implementation is based on https://github.com/JuliaLang/julia/blob/d29126a43ee289fc5ab8fcb3dc0e03f514605950/base/reshapedarray.jl#L119-L137

closes #220

cc: @jishnub

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 11, 2021

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.26%. Comparing base (b74ae55) to head (bf24c31).
⚠️ Report is 113 commits behind head on master.

Files with missing lines Patch % Lines
src/utils.jl 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   94.40%   94.26%   -0.15%     
==========================================
  Files           5        5              
  Lines         322      349      +27     
==========================================
+ Hits          304      329      +25     
- Misses         18       20       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

include("axes.jl")
include("utils.jl")
include("origin.jl")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is to let utils.jl know the OffsetAxisKnownLength and OffsetAxis.

For better compatibility to old and future Julia versions
@jishnub
Copy link
Copy Markdown
Member

jishnub commented Apr 11, 2021

Since this changes the axes of the reshaped array, this should perhaps be treated as a breaking change?

@johnnychen94
Copy link
Copy Markdown
Member Author

johnnychen94 commented Apr 11, 2021

Since we never successfully supported Colon in reshape before (had we?), I would say this is just a new feature. A minor version should be sufficient.

@jishnub
Copy link
Copy Markdown
Member

jishnub commented Apr 11, 2021

This works currently on master:

julia> A = zeros(1:2, 3:4)
2×2 OffsetArray(::Matrix{Float64}, 1:2, 3:4) with eltype Float64 with indices 1:2×3:4:
 0.0  0.0
 0.0  0.0

julia> reshape(A, 2:3, :)
2×2 OffsetArray(::Matrix{Float64}, 2:3, 1:2) with eltype Float64 with indices 2:3×1:2:
 0.0  0.0
 0.0  0.0

After this PR:

julia> reshape(A, 2:3, :)
2×2 OffsetArray(::Matrix{Float64}, 2:3, 3:4) with eltype Float64 with indices 2:3×3:4:
 0.0  0.0
 0.0  0.0

function Base.reshape(A::AbstractArray, inds::Tuple{OffsetAxis,Vararg{OffsetAxis}})
AR = reshape(A, map(_indexlength, inds))
return OffsetArray(AR, map(_offset, axes(AR), inds))
return OffsetArray(AR, _offset_reshape_uncolon(A, inds))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is equivalent to

    return OffsetArray(AR, map(_offset, axes(AR), _offset_reshape_uncolon(A, inds)))

By removing the map part, it calls another constructor.

# convert ranges to offsets
@eval @inline function $FT(A::AbstractArray, inds::Tuple{AbstractUnitRange,Vararg{AbstractUnitRange}})
_checkindices(A, inds, "indices")
# Performance gain by wrapping the error in a function: see https://github.com/JuliaLang/julia/issues/37558
throw_dimerr(lA, lI) = throw(DimensionMismatch("supplied axes do not agree with the size of the array (got size $lA for the array and $lI for the indices"))
lA = size(A)
lI = map(length, inds)
lA == lI || throw_dimerr(lA, lI)
$FT(A, map(_offset, axes(A), inds))
end

@johnnychen94
Copy link
Copy Markdown
Member Author

Ahhhh... Didn't realize that it worked before. I think we need triage here on which solution we should take: #220 or this. @mbauman @timholy

@johnnychen94 johnnychen94 requested review from mbauman and timholy April 11, 2021 12:50
@jishnub
Copy link
Copy Markdown
Member

jishnub commented Apr 11, 2021

Perhaps some of the other methods need to be altered too to get this to be consistent:

julia> reshape(OffsetArray(3:4, 2), :) # preserves offset
3:4 with indices 3:4

julia> reshape(OffsetArray(3:4, 2), :, 1) # does not preserve offset
2×1 reshape(::UnitRange{Int64}, 2, 1) with eltype Int64:
 3
 4

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