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

update the OffsetArray test helper #45387

Merged
merged 5 commits into from
Jun 11, 2022
Merged

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented May 20, 2022

Sync the OffsetArray test helper with the repo.

Copy link
Sponsor Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

The docstring and compat patch are not needed for test helper, but it doesn't matter much to include them.

This PR is required for #45371. As long as the CI is green, I believe it's okay to merge and update the test-only dependency.

@johnnychen94 johnnychen94 added test This change adds or pertains to unit tests status:merge me PR is reviewed. Merge when all tests are passing and removed status:merge me PR is reviewed. Merge when all tests are passing labels May 20, 2022
@@ -671,7 +671,7 @@ end
# issue #38627
@testset "overflow in mapreduce" begin
# at len = 16 and len = 1025 there is a change in codepath
for len in [0, 1, 15, 16, 1024, 1025, 2048, 2049]
for len in [1, 15, 16, 1024, 1025, 2048, 2049]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Since OffsetArrays more eagerly checks the overflow behavior, len=0 is indeed an undefined behavior so I agree to remove it. -- Specially handling length 0 array could also be problematic because user codes might do push!(parent(offset_vector), x).

This OffsetArray(Int[], typemax(Int)) issue is already caught in OffsetArrays codebase https://github.com/JuliaArrays/OffsetArrays.jl/blob/44f9b50c5428416c8baa76af882c820605c17082/test/runtests.jl#L518

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The length-0 case is also related to #45389

@jishnub
Copy link
Contributor Author

jishnub commented May 20, 2022

I don't really understand the reported ambiguities. Eg:

(reshape(A::Main.OffsetArrays.OffsetArray, inds::Tuple{Vararg{Union{Colon, Int64}}}) in Main.OffsetArrays at /home/jishnu/julia/test/testhelpers/OffsetArrays.jl:564,
reshape(parent::AbstractVector, ::Tuple{Colon}) in Base at reshapedarray.jl:116)

but there's

reshape(A::OffsetVector{T} where T, ::Tuple{Colon}) in Main.OffsetArrays at /home/jishnu/julia/test/testhelpers/OffsetArrays.jl:561

which should remove this ambiguity? Also, the methods are an exact copy from OffsetArarys.jl, using which I don't obtain these ambiguities.

@jishnub
Copy link
Contributor Author

jishnub commented May 23, 2022

Test failures seem unrelated at this point

@johnnychen94 johnnychen94 added the status:merge me PR is reviewed. Merge when all tests are passing label May 23, 2022
@vtjnash vtjnash merged commit 20584cc into JuliaLang:master Jun 11, 2022
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Jun 16, 2022
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.

None yet

4 participants