Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Jan 26, 2021

This fixes #128 going by the discussion in julia#37274 and in the spirit of #196.

Now

julia> a = OffsetArray(1:3, 0:2);

julia> b = @view a[0];

julia> b[]
1

julia> b[1]
1

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #197 (0b76276) into master (1263513) will decrease coverage by 0.70%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   98.93%   98.23%   -0.71%     
==========================================
  Files           5        5              
  Lines         283      284       +1     
==========================================
- Hits          280      279       -1     
- Misses          3        5       +2     
Impacted Files Coverage Δ
src/OffsetArrays.jl 97.90% <66.66%> (-1.03%) ⬇️
src/axes.jl 100.00% <0.00%> (ø)

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 1263513...0b76276. Read the comment docs.

@timholy
Copy link
Member

timholy commented Jan 26, 2021

Can we just put this fix in Base? Changing ::OffsetVector to ::AbstractVector? I don't see anything OffsetArrays-specific about this fix.

@jishnub
Copy link
Member Author

jishnub commented Jan 26, 2021

Yes the fix should go to Base, this PR is to ensure correct behavior on older versions of julia

@jishnub
Copy link
Member Author

jishnub commented Jan 26, 2021

Crossref : julia#39404

@timholy
Copy link
Member

timholy commented Jan 26, 2021

Sounds good. Let's wait for that to merge and then add this only on older Julia versions. The risk of method ambiguities seems low in this case, but if we don't have to...

@timholy
Copy link
Member

timholy commented Feb 3, 2021

Is it weird that codecov flags compute_linindex as untested?

@jishnub
Copy link
Member Author

jishnub commented Feb 3, 2021

Indeed, I don't really understand how this works. Since imposing the version bound seems to have upset codecov, is it that the coverage is being reported by the test on nightly?

@johnnychen94
Copy link
Member

johnnychen94 commented Feb 3, 2021

My best guess is the first coverage upload in a short period, which happens to be the Julia latest(1 minute) due to a large number of invalidation fixes. If we want it to be predictable, then we could add a version check in our CI settings:

- uses: codecov/codecov-action@v1
with:
file: lcov.info

@jishnub jishnub merged commit ee0fe71 into JuliaArrays:master Feb 4, 2021
@jishnub jishnub deleted the linindex branch February 4, 2021 08:41
@timholy
Copy link
Member

timholy commented Feb 6, 2021

Thanks again for fixing this!

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.

LinearIndexing for a zero-dimensional view of an OffsetVector doesn't work as expected

3 participants