summary simplification, test cases, and eager checks #28

johnnychen94 · 2021-08-24T16:52:47Z

Because the current test fails on the master version, I submit all commits in one single PR.

Changes:

simplify summary show of Align and Slices
add comprehensive testsets and move doctest to document CI (with strict mode)
some style changes
support linear indexing for both Align and Slices (closes Slices doesn't support linear indices #27 )

Also allow incomplete `alongs::TypedBool...` input, with tailing missing values default to `False()`

add JuliennedArrays to docs/Project.toml, ven though we must do `pkg> dev .` to use local development version of it.

johnnychen94 · 2021-08-24T17:11:36Z

src/JuliennedArrays.jl

+export Slices, Align
+export True, False


Actually, I think these names are too common to be exported.

Who uses them?

Slice exists in Julia Base with a completely different meaning, I feel that's quite ambiguous to have Slices meaning the others. SliceView and AlignView are more accurate IMO.

True/False are too common names here and I think they should be either implementation details or preserved by Base. But apparently, people just invent their own without much effort:

https://github.com/SciML/Static.jl/blob/0f293e94fcbfbc812cdeaa796bd549b4ec2bc1ce/src/bool.jl#L2-L11

I'd be fine with SlicesView and AlignView. I added the s because it's many slices. I think post JuliaLang/julia#40561 we might be able to just use true and false instead with no extra performance cost as long as they are constant, but someone would need to verify that.

bramtayl · 2021-08-25T02:29:05Z

src/JuliennedArrays.jl

-    else
-        1
-    end
+@inline axis_or_1(switch, axis) = untyped(switch) ? axis : 1


I tend not to like to use the ternary operator. It's inconvenient if you want to add more lines to the if statement

The idea here is to only use short-form function definitions when they can be fit into one line.

https://github.com/invenia/BlueStyle#method-definitions

I have to admit that the current codebase requires a little bit of extra effort to read even if I've been used to Julia for years. I didn't make style changes to every line of codes that I feel not so comfortable to read, because they're mostly unrelated to this PR. If you insist we can revert this one.

Eh, I don't really care that much. Just the way I do things

It's up to you. IMHO the codebase should be clean and simplified so that others could jump in and make contributions without much mental efforts. If that codebase or code style is too opinioned then it's setting a high wall and blocks people.

bramtayl · 2021-08-25T02:31:56Z

Hi! Thanks for the contribution! I think most of the changes look good. Could you separate out the changes to the constructors to a separate pull request? That's the part I'm hesitant about

bramtayl · 2021-08-25T02:45:18Z

src/JuliennedArrays.jl

    alongs::Alongs
+    function Align{T,N,S,A}(slices::S, alongs::A) where {T,N,S,A}
+        sz = size(first(slices))
+        all(x->sz==size(x), slices) || throw(ArgumentError("All sizes of slices should be the same."))


This seems like it could have a significant performance cost? Maybe annotate with @boundscheck?

I don't think @boundscheck would help here as it's internally managed by all.

Some of the difficulty comes from the fact that slices is a vector and not a tuple. If it's a tuple the check would be quicker to run.

It turns out that the main overhead here is map(size, slices):

julia> Xs = Slices(rand(2, 3, 4, 5), 1); julia> @btime map(size, $Xs); 77.824 ns (1 allocation: 576 bytes)

I've commented this check so that no significant runtime overhead is introduced. But I do believe we need this check to prevent undefined behaviors, so I mark it as "TODO".

Maybe I don't understand how @boundscheck works then. My understanding was that if Align is called with @inbounds, it will skip evaulating the entire statement?

No, if I understand it correctly, you need to add Base.@propagate_inbounds annotation to pass through @inbounds check, so for example, it should be

- @inline getindex(slices::Slices{T,N}, indices::Vararg{Int,N}) where {T,N} = + Base.@propagate_inbounds getindex(slices::Slices{T,N}, indices::Vararg{Int,N}) where {T,N} = view(slices.whole, slice_index(slices, indices)...)

so that @inbounds X[i] will become @inbounds view(...). Currently, it will only be view(...).

bramtayl · 2021-08-25T02:47:28Z

allow incomplete Slices construction: Slices(Xs, True()) is equivalent to Slices(Xs, True(), False(), ... False())

I'm not sure about this. Slices(Xs, True()) seems unclear for many-dimensional arrays

bramtayl · 2021-08-25T02:50:14Z

test/runtests.jl

+        @test Xs[1, 3] == X[:, 1, :, 3] # test cartesian indexing
+
+        # type is not inferrable for integer alongs
+        RT = Base.return_types(Slices, (typeof(X), Int, Int))[1]


When I wrote it I took special care to make sure that this would be inferable if constant propagation kicks in

Unless wrapped with Val, I don't think it's inferrable if the input is an integer whose value is not known at compile time.

This is the behavior on master branch:

julia> @inferred Slices(rand(2, 3), 2) ERROR: return type Slices{SubArray{Float64, 1, Matrix{Float64}, Tuple{Int64, Base.OneTo{Int64}}, true}, 1, Matrix{Float64}, Tuple{False, True}} does not match inferred return type Slices{Item, Dimensions, Matrix{Float64}, Alongs} where {Item, Dimensions, Alongs}

Right, but @inferred (x -> Slices(x, 2))(rand(2, 3)) or something like that should hopefully work

bramtayl · 2021-08-25T02:59:43Z

src/JuliennedArrays.jl

+        sz = size(first(slices))
+        all(x->sz==size(x), slices) || throw(ArgumentError("All sizes of slices should be the same."))
+        length(alongs) == N || throw(DimensionMismatch("The total dimension $(N) is expected to be the sum of inner dimension $(length(sz)) and outer dimension $(length(alongs))"))
+        inner_dimensions = mapreduce(isequal(True()), +, alongs)


It would be good to verify that these checks can be done by constant-propagation at compile-time

For slices whose size isn't known prior, it's unable to optimize it away at compile time.
For alongs whose value is stored as types, it's mostly optimized, check the benchmark for Align #28 (comment)

I used True instead of true on purpose to make sure everything stays in the type-domain. So this is the kind of thing that the compiler should be able to figure out.

johnnychen94

Will remove the incomplete construction and check the construction overhead later.

My guess is, unless we choose to store slices in NTuple, it's not so easy to make the constructor fast.

johnnychen94 · 2021-08-25T03:57:43Z

test/runtests.jl

+        @test Xs[1, 3] == X[:, 1, :, 3] # test cartesian indexing
+
+        # type is not inferrable for integer alongs
+        RT = Base.return_types(Slices, (typeof(X), Int, Int))[1]


Unless wrapped with Val, I don't think it's inferrable if the input is an integer whose value is not known at compile time.

This is the behavior on master branch:

julia> @inferred Slices(rand(2, 3), 2) ERROR: return type Slices{SubArray{Float64, 1, Matrix{Float64}, Tuple{Int64, Base.OneTo{Int64}}, true}, 1, Matrix{Float64}, Tuple{False, True}} does not match inferred return type Slices{Item, Dimensions, Matrix{Float64}, Alongs} where {Item, Dimensions, Alongs}

johnnychen94 · 2021-08-25T03:58:02Z

src/JuliennedArrays.jl

    alongs::Alongs
+    function Align{T,N,S,A}(slices::S, alongs::A) where {T,N,S,A}
+        sz = size(first(slices))
+        all(x->sz==size(x), slices) || throw(ArgumentError("All sizes of slices should be the same."))


I don't think @boundscheck would help here as it's internally managed by all.

Some of the difficulty comes from the fact that slices is a vector and not a tuple. If it's a tuple the check would be quicker to run.

johnnychen94 · 2021-08-25T04:05:18Z

src/JuliennedArrays.jl

+export Slices, Align
+export True, False


Slice exists in Julia Base with a completely different meaning, I feel that's quite ambiguous to have Slices meaning the others. SliceView and AlignView are more accurate IMO.

True/False are too common names here and I think they should be either implementation details or preserved by Base. But apparently, people just invent their own without much effort:

https://github.com/SciML/Static.jl/blob/0f293e94fcbfbc812cdeaa796bd549b4ec2bc1ce/src/bool.jl#L2-L11

johnnychen94 · 2021-08-25T06:30:19Z

There are several other things that can be improved, but for simplicity, I'd like to leave them as future PRs:

For type inferrable and compile-time optimization. Maybe introduce Static.jl, it has a more sophisticated design and also supports common number operations. The drawback is the loading time is 10x larger than this package. (using Static gives 0.05s while using JuliennedArrays gives 0.004s on my machine)
There can be some optimizations on Align(Slices(...)...) and Slices(Align(...)...).

For runtime overhead, I wouldn't say it's zero overhead, but I think they're necessary checks.

using BenchmarkTools
using JuliennedArrays

# Align

Xs = [rand(3, 4) for _ in 1:8];
Xs = reshape(Xs, 2, 4);

@btime Align($Xs, 1, 3);
# PR: 998.615 ns (2 allocations: 48 bytes)
# master: 1.005 μs (2 allocations: 48 bytes)
@btime Align($Xs, $(True()), $(False()), $(True()), $(False()));
# PR: 1.475 ns (0 allocations: 0 bytes)
# master: 1.431 ns (0 allocations: 0 bytes)

X = rand(2, 3, 4, 5);
Xs = Slices(X, True(), False(), True(), False());

@btime Align($Xs, 1, 3);
# PR: 1.008 μs (3 allocations: 64 bytes)
# master: 995.364 ns (3 allocations: 64 bytes)
@btime Align($Xs, $(True()), $(False()), $(True()), $(False()));
# PR: 6.798 ns (0 allocations: 0 bytes)
# master: 1.432 ns (0 allocations: 0 bytes)

# Slices

X = rand(2, 3, 4, 5);
@btime Slices($X, 1, 3);
# PR: 2.731 μs (19 allocations: 816 bytes)
# master: 2.745 μs (18 allocations: 784 bytes)
@btime Slices($X, $(True()), $(False()), $(True()), $(False()));
# PR: 1.620 ns (0 allocations: 0 bytes)
# master: 1.432 ns (0 allocations: 0 bytes)

Xs = [rand(3, 4) for _ in 1:8];
Xs = reshape(Xs, 2, 4);
X = Align(Xs, True(), False(), True(), False());
@btime Slices($X, 1, 3);
# PR: 2.923 μs (20 allocations: 832 bytes)
# master: 2.871 μs (19 allocations: 800 bytes)
@btime Slices($X, $(True()), $(False()), $(True()), $(False()));
# PR: 1.831 μs (18 allocations: 768 bytes)
# master: 1.742 μs (18 allocations: 768 bytes)

johnnychen94 · 2021-08-25T19:27:28Z

I'm not sure if I get it right. Is there any change request on the current codes or are those comments just questions?

bramtayl · 2021-08-25T20:41:55Z

Mostly just comments. If you separate out the changes to the constructor/things that might affect performance, I can merge that, and then we can work on reducing the performance cost of the rest? I know it's relatively minor but I think we can get rid of most of the cost.

johnnychen94

@bramtayl I'm running out of time before the semester begins (Sep 1) and still have other things to take care of, so I'll leave everything else to you.

The unresolved comments are still quite unclear to me, if you have concerns about the runtime check overhead and if you have ideas about how to give better hints to the compilers I'd be happy to see how that's applied.

For the moment I just commented out all the construction-time checks, if you want a minimal PR differences and a clean codebase you can also choose to delete them.

johnnychen94 · 2021-08-26T05:46:56Z

src/JuliennedArrays.jl

-    else
-        1
-    end
+@inline axis_or_1(switch, axis) = untyped(switch) ? axis : 1


It's up to you. IMHO the codebase should be clean and simplified so that others could jump in and make contributions without much mental efforts. If that codebase or code style is too opinioned then it's setting a high wall and blocks people.

bramtayl · 2021-08-26T16:11:20Z

Ok, well thanks for the contribution! I'll take a stab at increasing the performance of the commented code and then tag you in the PR when I get to it.

johnnychen94 added 3 commits August 25, 2021 00:18

add testset and eager argument checks on construction

4fe67f4

Also allow incomplete `alongs::TypedBool...` input, with tailing missing values default to `False()`

move doctest to document CI

f756d68

add JuliennedArrays to docs/Project.toml, ven though we must do `pkg> dev .` to use local development version of it.

simplify summary show

93035a8

johnnychen94 mentioned this pull request Aug 24, 2021

add convenient functions stack/unstack #26

Open

support linear indexing

63e7746

johnnychen94 commented Aug 24, 2021

View reviewed changes

johnnychen94 mentioned this pull request Aug 24, 2021

Iteration failure with CartesianIndex #17

Closed

bramtayl reviewed Aug 25, 2021

View reviewed changes

johnnychen94 commented Aug 25, 2021

View reviewed changes

johnnychen94 added 2 commits August 25, 2021 13:47

revert incomplete constructors

9b25f18

comment out size checks

3644e35

comment out all the safety checks

c0128bd

johnnychen94 commented Aug 26, 2021

View reviewed changes

comment out 1 more line

9a39d67

bramtayl merged commit fb9bf0a into bramtayl:master Aug 26, 2021

johnnychen94 deleted the jc/testsets branch August 26, 2021 16:36

summary simplification, test cases, and eager checks #28

summary simplification, test cases, and eager checks #28

Uh oh!

Conversation

johnnychen94 commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramtayl Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramtayl commented Aug 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramtayl commented Aug 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnychen94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnychen94 commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnnychen94 commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bramtayl commented Aug 25, 2021

Uh oh!

johnnychen94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramtayl commented Aug 26, 2021

Uh oh!

Reviewers

johnnychen94 commented Aug 24, 2021 •

edited

Loading

bramtayl Aug 25, 2021 •

edited

Loading

johnnychen94 Aug 25, 2021 •

edited

Loading

johnnychen94 Aug 25, 2021 •

edited

Loading

johnnychen94 commented Aug 25, 2021 •

edited

Loading

johnnychen94 commented Aug 25, 2021 •

edited

Loading