Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Apr 29, 2021

Tests with Base.IdentityUnitRange were missed out in #39896, would be good to include these as well

@jishnub jishnub changed the title Add tests for indexing with ranges Add tests for indexing arrays with ranges Apr 29, 2021
@jishnub jishnub changed the title Add tests for indexing arrays with ranges Add tests for indexing arrays with IdentityUnitRanges Apr 29, 2021
Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

Small indentation nit, but otherwise LGTM.

@dkarrasch dkarrasch added the test This change adds or pertains to unit tests label Apr 29, 2021
@vtjnash vtjnash closed this Jun 19, 2021
@vtjnash vtjnash reopened this Jun 19, 2021
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jun 19, 2021
@vtjnash vtjnash force-pushed the offsetvectorindexing branch from 5cefdcd to 1b54669 Compare July 1, 2021 16:18
@vtjnash vtjnash removed the merge me PR is reviewed. Merge when all tests are passing label Jul 1, 2021
@vtjnash vtjnash marked this pull request as draft July 1, 2021 16:19
@vtjnash
Copy link
Member

vtjnash commented Jul 1, 2021

Tried to merge this, but the tests failed. See commit message of 1b54669

@vtjnash
Copy link
Member

vtjnash commented Jul 1, 2021

Possibly broken by #40660?

@jishnub jishnub force-pushed the offsetvectorindexing branch from 1b54669 to 77057c3 Compare July 4, 2021 09:00
@jishnub
Copy link
Member Author

jishnub commented Jul 5, 2021

Indeed, should be fixed now. Tests for IdentityUnitRange(::UnitRange) only work with OffsetArrays loaded, and were added by #40660 anyway. Test failures on windows seem unrelated

@vtjnash vtjnash marked this pull request as ready for review July 13, 2021 16:58
@vtjnash vtjnash merged commit 20c0baf into JuliaLang:master Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test This change adds or pertains to unit tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants