Make partition support non-1-indexed vector. #40830

N5N3 · 2021-05-15T10:28:59Z

For a high dimension array, at least OffsetArrays.jl return a 1-indexed vector after reshape, so the current implementation seems ok.

Since the view of Base's Range types has been mapped to getindex correctly, so I think we don't need a separate routine.

For a high dimension array, at least `OffsetArrays.jl` return a 1-indexed vector after reshape, so the current implementation seems ok. I believe that the view of Base's Range types has been mapped to `getindex` correctly, so I think we don't need a separate routine.

johnnychen94

Tests are needed to make sure that this PR fixes something. There might be other types, but at least to test the following ones:

Base.IdentityUnitRange
OffsetArrays.OffsetArray
OffsetArrays.IdOffsetRange

N5N3 · 2021-05-15T10:50:20Z

base/iterators.jl

@@ -1171,10 +1171,10 @@ function length(itr::PartitionIterator)
    return cld(l, itr.n)
 end

-function iterate(itr::PartitionIterator{<:AbstractRange}, state=1)
-    state > length(itr.c) && return nothing
+function iterate(itr::PartitionIterator{<:AbstractVector}, state = firstindex(itr.c))


I'm not clear with "invalid", if we add a dispatch for PartitionIterator{<:AbstractRange} , then everything is the same except for replacing view with getindex.

on 1.6.1

julia> UnitRange <: AbstractRange <: AbstractVector true

something changed in dev?

johnnychen94 · 2021-05-15T10:35:45Z

base/iterators.jl

    r = min(state + itr.n - 1, length(itr.c))
-    return @inbounds itr.c[state:r], r + 1
+    return @inbounds view(itr.c, state:r), r + 1


If this change doesn't make a difference, there's no need to change it.

I think the view makes it consistent with the docstring

I am a bit hesitant about this, at least as part of this PR. Since this is supposed to just fix a bug can we remove it for now and perhaps propose that change in a separate PR?

johnnychen94 · 2021-05-15T10:39:08Z

base/iterators.jl

    r = min(state + itr.n - 1, length(itr.c))
-    return @inbounds itr.c[state:r], r + 1
+    return @inbounds view(itr.c, state:r), r + 1
 end

 function iterate(itr::PartitionIterator{<:AbstractArray}, state=1)


Looks like the AbstractArray method can be improved, too?

Hmmm, actually, no need. Because here it's using LinearIndexing rules, which always assume 1-based.

N5N3 · 2021-05-15T10:50:20Z

base/iterators.jl

@@ -1171,10 +1171,10 @@ function length(itr::PartitionIterator)
    return cld(l, itr.n)
 end

-function iterate(itr::PartitionIterator{<:AbstractRange}, state=1)
-    state > length(itr.c) && return nothing
+function iterate(itr::PartitionIterator{<:AbstractVector}, state = firstindex(itr.c))


I'm not clear with "invalid", if we add a dispatch for PartitionIterator{<:AbstractRange} , then everything is the same except for replacing view with getindex.

add test

move to offsetarray.jl

remove white-space

johnnychen94 · 2021-05-15T11:37:23Z

test/offsetarray.jl

@@ -786,3 +786,8 @@ end
        @test b[i] == a[r[i]]
    end
 end
+
+@testset "proper patition for non-1-indexed vector" begin
+    @test partition(OffsetArray(1:10,10), 5) |> collect == [1:5,6:10] # OffsetVector


and also partition(OffsetArray(collect(1:10),10), 5) |> collect. It currently sigfaults with improper index.

johnnychen94 · 2021-05-15T11:38:46Z

base/iterators.jl

-function iterate(itr::PartitionIterator{<:AbstractRange}, state=1)
-    state > length(itr.c) && return nothing
+function iterate(itr::PartitionIterator{<:AbstractVector}, state = firstindex(itr.c))
+    state > lastindex(itr.c) && return nothing
    r = min(state + itr.n - 1, length(itr.c))


Suggested change

r = min(state + itr.n - 1, length(itr.c))

r = min(state + itr.n - 1, lastindex(itr.c))

johnnychen94 · 2021-05-15T11:39:54Z

base/iterators.jl

    r = min(state + itr.n - 1, length(itr.c))
-    return @inbounds itr.c[state:r], r + 1
+    return @inbounds view(itr.c, state:r), r + 1
 end

 function iterate(itr::PartitionIterator{<:AbstractArray}, state=1)


Hmmm, actually, no need. Because here it's using LinearIndexing rules, which always assume 1-based.

fix

fix the depend

johnnychen94

The current version works for high-dimensional OffsetArray because linear indexing uses 1-based indexing rules. However, I still recommend adding the missing test for it (2d OffsetArray).

test/offsetarray.jl

Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>

Add offsetmatrix

N5N3 · 2021-05-16T05:17:00Z

The current version works for high-dimensional OffsetArray because linear indexing uses 1-based indexing rules. However, I still recommend adding the missing test for it (2d OffsetArray).

Well, test for OffsetMatrix has been added.
The failue on Linux64 seems to be brought by a Cholesky factorization of non-positive definite matrix. (So the corresponding testset might be improved?)

@simeonschaub

follow @simeonschaub's advice to retain the dispatch for AbstractRange

N5N3 · 2021-05-20T01:12:40Z

@simeonschaub the dispatch for AbstractRange has been retained.
（I hope I understand your advice correctly)

simeonschaub · 2021-05-20T07:09:51Z

Sorry if I missed something above, but why does AbstractVector need a separate method from AbstractArray here? I think it should be fine to remove the special case for Partition{<:AbstractVector} and instead just use firstindex and lastindex in the generic implementation for Partition{<:AbstractArray}.

remove the the special case for abstractvector

N5N3 · 2021-05-20T07:21:16Z

That's right, didn't notice that.

simeonschaub

Great, looks good now! Thanks for the contribution! (BTW, there is no need to close PRs if you are just pushing a new commit)

For a high dimension array, at least `OffsetArrays.jl` return a 1-indexed vector after reshape, so the current implementation seems ok. I believe that the view of Base's Range types has been mapped to `getindex` correctly, so I think we don't need a separate routine. Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>

johnnychen94 reviewed May 15, 2021

View reviewed changes

N5N3 commented May 15, 2021

View reviewed changes

N5N3 added 5 commits May 15, 2021 19:19

add test

e9ff809

add test

Update iterators.jl

97fa632

move to offsetarray.jl

Update offsetarray.jl

5eb64b1

Update iterators.jl

af7e493

remove white-space

Update offsetarray.jl

3ace109

johnnychen94 reviewed May 15, 2021

View reviewed changes

N5N3 added 2 commits May 15, 2021 19:44

Update iterators.jl

d87b631

fix

Update offsetarray.jl

f1e8f17

fix the depend

johnnychen94 approved these changes May 15, 2021

View reviewed changes

test/offsetarray.jl Show resolved Hide resolved

johnnychen94 added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels May 15, 2021

N5N3 and others added 3 commits May 16, 2021 08:46

Update test/offsetarray.jl

df2afa7

Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>

Update offsetarray.jl

b5b3bdd

Add offsetmatrix

Update offsetarray.jl

8962a99

johnnychen94 added the merge me PR is reviewed. Merge when all tests are passing label May 16, 2021

simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label May 19, 2021

N5N3 closed this May 20, 2021

N5N3 added 2 commits May 20, 2021 09:00

Merge branch 'JuliaLang:master' into master

5c5316e

restore the dispatch for AbstractRange

ad54565

follow @simeonschaub's advice to retain the dispatch for AbstractRange

N5N3 reopened this May 20, 2021

N5N3 closed this May 20, 2021

N5N3 added 2 commits May 20, 2021 15:15

Merge branch 'JuliaLang:master' into master

31b4b51

Update iterators.jl

980e4c6

remove the the special case for abstractvector

N5N3 reopened this May 20, 2021

simeonschaub approved these changes May 20, 2021

View reviewed changes

simeonschaub merged commit 64940ec into JuliaLang:master May 21, 2021

LilithHafner mentioned this pull request Oct 23, 2024

Correction of lcm() for Array arguments #56113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make partition support non-1-indexed vector. #40830

Make partition support non-1-indexed vector. #40830

N5N3 commented May 15, 2021 •

edited

Loading

johnnychen94 left a comment

N5N3 May 15, 2021 •

edited

Loading

This comment was marked as resolved.

N5N3 May 15, 2021

This comment was marked as resolved.

johnnychen94 May 15, 2021

jishnub May 15, 2021

simeonschaub May 19, 2021

johnnychen94 May 15, 2021

johnnychen94 May 15, 2021

N5N3 May 15, 2021 •

edited

Loading

johnnychen94 May 15, 2021

johnnychen94 May 15, 2021

johnnychen94 May 15, 2021

johnnychen94 left a comment

N5N3 commented May 16, 2021

N5N3 commented May 20, 2021

simeonschaub commented May 20, 2021

N5N3 commented May 20, 2021

simeonschaub left a comment

	r = min(state + itr.n - 1, length(itr.c))
	r = min(state + itr.n - 1, lastindex(itr.c))

Make partition support non-1-indexed vector. #40830

Make partition support non-1-indexed vector. #40830

Conversation

N5N3 commented May 15, 2021 • edited Loading

johnnychen94 left a comment

Choose a reason for hiding this comment

N5N3 May 15, 2021 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N5N3 May 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnychen94 left a comment

Choose a reason for hiding this comment

N5N3 commented May 16, 2021

N5N3 commented May 20, 2021

simeonschaub commented May 20, 2021

N5N3 commented May 20, 2021

simeonschaub left a comment

Choose a reason for hiding this comment

N5N3 commented May 15, 2021 •

edited

Loading

N5N3 May 15, 2021 •

edited

Loading

N5N3 May 15, 2021 •

edited

Loading