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.

Sign up for GitHub

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

Jump to bottom

Adding tests for AbstractArrayMath.jl #56773

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Adding tests for AbstractArrayMath.jl #56773

wants to merge 6 commits into from

Conversation

Priynsh
Copy link
Contributor

@Priynsh Priynsh commented Dec 7, 2024

added tests for lines 7, 137-154 (insertdims function) from base/abstractarraymath.jl

@nsajko nsajko added test This change adds or pertains to unit tests arrays [a, r, r, a, y, s] labels Dec 7, 2024
@nsajko
Copy link
Contributor

nsajko commented Dec 7, 2024

Probably want a more descriptive PR title and commit message.

@Priynsh Priynsh changed the title Update abstractarray.jl Adding tests for abstractmath Dec 7, 2024
@Priynsh Priynsh changed the title Adding tests for abstractmath Adding tests for AbstractArrayMath.jl Dec 7, 2024
LilithHafner
LilithHafner reviewed Dec 7, 2024
View reviewed changes
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks for contributing tests!

I can confirm that codecov reports both this isreal method and all of insertdims as untested. I don't see anywhere in test/ where this isreal method is tested but I do see that insertdims is tested here:

julia/test/arrayops.jl

Lines 312 to 338 in 6cb9f04

a = rand(8, 7)
@test @inferred(insertdims(a, dims=1)) == @inferred(insertdims(a, dims=(1,))) == reshape(a, (1, 8, 7))
@test @inferred(insertdims(a, dims=3)) == @inferred(insertdims(a, dims=(3,))) == reshape(a, (8, 7, 1))
@test @inferred(insertdims(a, dims=(1, 3))) == reshape(a, (1, 8, 1, 7))
@test @inferred(insertdims(a, dims=(1, 2, 3))) == reshape(a, (1, 1, 1, 8, 7))
@test @inferred(insertdims(a, dims=(1, 4))) == reshape(a, (1, 8, 7, 1))
@test @inferred(insertdims(a, dims=(1, 3, 5))) == reshape(a, (1, 8, 1, 7, 1))
@test @inferred(insertdims(a, dims=(1, 2, 4, 6))) == reshape(a, (1, 1, 8, 1, 7, 1))
@test @inferred(insertdims(a, dims=(1, 3, 4, 6))) == reshape(a, (1, 8, 1, 1, 7, 1))
@test @inferred(insertdims(a, dims=(1, 4, 6, 3))) == reshape(a, (1, 8, 1, 1, 7, 1))
@test @inferred(insertdims(a, dims=(1, 3, 5, 6))) == reshape(a, (1, 8, 1, 7, 1, 1))
@test_throws ArgumentError insertdims(a, dims=(1, 1, 2, 3))
@test_throws ArgumentError insertdims(a, dims=(1, 2, 2, 3))
@test_throws ArgumentError insertdims(a, dims=(1, 2, 3, 3))
@test_throws UndefKeywordError insertdims(a)
@test_throws ArgumentError insertdims(a, dims=0)
@test_throws ArgumentError insertdims(a, dims=(1, 2, 1))
@test_throws ArgumentError insertdims(a, dims=4)
@test_throws ArgumentError insertdims(a, dims=6)
# insertdims and dropdims are inverses
b = rand(1,1,1,5,1,1,7)
for dims in [1, (1,), 2, (2,), 3, (3,), (1,3), (1,2,3), (1,2), (1,3,5), (1,2,5,6), (1,3,5,6), (1,3,5,6), (1,6,5,3)]
@test dropdims(insertdims(a; dims); dims) == a
@test insertdims(dropdims(b; dims); dims) == b
end

Codecov does not currently display coverage results of this PR.

test/abstractarray.jl Outdated Show resolved Hide resolved
test/abstractarray.jl Outdated Show resolved Hide resolved
test/abstractarray.jl
F = [MyReal(1.0), MyReal(2.0)]
@test isreal(F) == true
G = ["a", "b", "c"]
@test_throws MethodError isreal(G)
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate you testing this failure case as well.

Priynsh and others added 3 commits December 8, 2024 15:22
@Priynsh @LilithHafner
Update test/abstractarray.jl
d2e0c40 
make stricter test

Co-authored-by: Lilith Orion Hafner <[email protected]>
@Priynsh @LilithHafner
Update test/abstractarray.jl
57a7c47 
Co-authored-by: Lilith Orion Hafner <[email protected]>
@Priynsh
Update abstractarray.jl
e7199d6 
removing redundant tests already done before
@Priynsh
Copy link
Contributor Author

Priynsh commented Dec 8, 2024

thanks for such a detailed feedback! It really helps :) I had missed the insertdims test which were already present in the code, but they are not complete. So I have removed any redundancies for the same. Should i move all the insertdims test be together in arrayops? or is this okay?

@Priynsh Priynsh marked this pull request as ready for review December 8, 2024 10:05
@Priynsh Priynsh requested a review from LilithHafner December 8, 2024 10:05
@LilithHafner
Copy link
Member

Yes, please. All the insertdims tests should be together.

@Priynsh
Copy link
Contributor Author

Priynsh commented Dec 8, 2024

I have added my tests in the same place as well. making a test-set for the same :)

LilithHafner
LilithHafner approved these changes Dec 8, 2024
View reviewed changes
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks!

@LilithHafner LilithHafner added the merge me PR is reviewed. Merge when all tests are passing label Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LilithHafner LilithHafner LilithHafner approved these changes

Assignees
No one assigned
Labels
arrays [a, r, r, a, y, s] merge me PR is reviewed. Merge when all tests are passing test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Priynsh @nsajko @LilithHafner