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

propagate first and last for OffsetRanges #258

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Aug 22, 2021

Propagate first and last to the parent for OffsetArray(::AbstractRange). On master step was propagated anyway as this is not defined for abstractarrays in general. The current change is an optimization as it gets around the need to index into the array to find the first element.

The first and last elements for ranges are often obtained by explicitly accessing fields, so I don't think that it's possible to obtain these for wrapper types by using traits.

I'm not sure if this provides an immediate real-world performance benefit, so perhaps we may leave this open for now. I had created this keeping something like JuliaLang/julia#41945 in mind. If iterate uses traits then we may require a rapid evaluation of first and last elements for OffsetRanges to be as performant as ranges.

@codecov
Copy link

codecov bot commented Aug 22, 2021

Codecov Report

Merging #258 (9b9b7f0) into master (f3fad96) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #258   +/-   ##
=======================================
  Coverage   95.41%   95.41%           
=======================================
  Files           5        5           
  Lines         436      436           
=======================================
  Hits          416      416           
  Misses         20       20           
Impacted Files Coverage Δ
src/OffsetArrays.jl 98.29% <100.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 f3fad96...9b9b7f0. Read the comment docs.

@jishnub jishnub marked this pull request as draft August 27, 2021 14:03
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.

1 participant