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

AbstractUnitRange for BitArray/BitVector indexing #41810

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Aug 6, 2021

Several methods could be generalized to work with AbstractUnitRange rather than just UnitRange. There are now several AbstractUnitRange subtypes:

julia> subtypes(AbstractUnitRange)
4-element Vector{Any}:
 Base.IdentityUnitRange
 Base.OneTo
 Base.Slice
 UnitRange

This pull request adapts several methods to work on BitArray / BitVector and AbstractUnitRange.

@johnnychen94
Copy link
Sponsor Member

johnnychen94 commented Aug 6, 2021

Can you also share more information about this change? For example, what was wrong with the previous status. The code change itself doesn't explain the background very well.

It's also a good practice to have associated test cases with every non-trivial functionality change.

I also see that you just opened the other two similar PR #41805, #41807 and #41809. The idea behind those changes are also uncleared to me.

@mkitti
Copy link
Contributor Author

mkitti commented Aug 7, 2021

The impetus for the series of PRs stems from
#39241 (comment)

Basically if we have more Base.OneTo around is the Base ecosystem ready for it?

@johnnychen94
Copy link
Sponsor Member

For that, I don't have any objection. But please follow the previous PR #39896 as an example, prepare for more explanation and sufficient test cases so that when someone with privilege jumps in, he doesn't need to ask you to do these and we don't need to wait for one more round of review.

@mkitti
Copy link
Contributor Author

mkitti commented Aug 7, 2021

Thanks @johnnychen94 . I just haven't had time to write it up just yet and mainly focusing on that range PR at the moment. This PR and the other related ones are mainly just to scout ahead for issues that may create in the future. There seems to be some complicated issues with the string part.

@mkitti mkitti marked this pull request as draft August 7, 2021 20:30
@mkitti mkitti changed the title AbstractUnitRange for BitArray indexing AbstractUnitRange for BitArray/BitVector indexing Aug 7, 2021
@mkitti mkitti marked this pull request as ready for review August 16, 2021 22:48
@vtjnash vtjnash merged commit 1976045 into JuliaLang:master Aug 20, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
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.

3 participants