Skip to content

Generalize Base._cat to non-Val, typed Base._cat_t and implement typed_hcat, typed_vcat, typed_hvcat, typed_hvncat #163

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

Merged
merged 20 commits into from
Oct 6, 2024

Conversation

mofeing
Copy link
Collaborator

@mofeing mofeing commented Oct 4, 2024

It's not yet finished, but this should fix #148

@mofeing mofeing requested a review from avik-pal October 4, 2024 18:59
Copy link
Contributor

github-actions bot commented Oct 4, 2024

Benchmark Results

main 601c867... main/601c867cf68ebb...
comptime/basics/2D sum 29.5 ± 1.2 ms 26.6 ± 1.1 ms 1.11
comptime/basics/Basic grad cos 0.0484 ± 0.0016 s 0.0461 ± 0.00095 s 1.05
comptime/basics/cos.(x) 0.0344 ± 0.00095 s 31.2 ± 0.63 ms 1.1
comptime/lux neural networks/ViT base 5.8 s 5.54 s 1.05
comptime/lux neural networks/ViT tiny 6.26 s 5.75 s 1.09
comptime/lux neural networks/vgg11 bn=false 0.39 ± 0.024 s 0.386 ± 0.017 s 1.01
comptime/lux neural networks/vgg13 bn=false 0.502 ± 0.025 s 0.439 ± 0.016 s 1.14
comptime/lux neural networks/vgg16 bn=false 0.582 ± 0.011 s 0.526 ± 0.0072 s 1.11
comptime/lux neural networks/vgg19 bn=false 0.661 ± 0.04 s 0.592 ± 0.016 s 1.12
runtime/lux neural networks/ViT base (compiled) 6.42 s 6.39 s 1
runtime/lux neural networks/ViT tiny (compiled) 1.7 s 1.68 s 1.01
runtime/lux neural networks/vgg11 bn=false (compiled) 2.1 s 2.13 s 0.988
runtime/lux neural networks/vgg13 bn=false (compiled) 3.01 s 2.97 s 1.01
runtime/lux neural networks/vgg16 bn=false (compiled) 3.75 s 3.72 s 1.01
runtime/lux neural networks/vgg19 bn=false (compiled) 4.56 s 4.52 s 1.01
time_to_load 1.54 ± 0.018 s 1.43 ± 0.0063 s 1.07

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Reactant.jl Benchmarks

Benchmark suite Current: 601c867 Previous: 9904590 Ratio
ViT base (256 x 256 x 3 x 32)/forward/CUDA/Reactant 1322553433 ns 1349734013 ns 0.98
ViT base (256 x 256 x 3 x 32)/forward/CUDA/Lux 228722429 ns 206256598 ns 1.11
ViT base (256 x 256 x 3 x 32)/forward/CPU/Reactant 5172361198 ns 5640616179 ns 0.92
ViT base (256 x 256 x 3 x 32)/forward/CPU/Lux 13577737011 ns 21088566845 ns 0.64
ViT small (256 x 256 x 3 x 4)/forward/CUDA/Reactant 1282941637.5 ns 1271785039.5 ns 1.01
ViT small (256 x 256 x 3 x 4)/forward/CUDA/Lux 8852342 ns 8622303 ns 1.03
ViT small (256 x 256 x 3 x 4)/forward/CPU/Reactant 1620252176 ns 1620712407 ns 1.00
ViT small (256 x 256 x 3 x 4)/forward/CPU/Lux 2243369927 ns 2515728657 ns 0.89
ViT tiny (256 x 256 x 3 x 32)/forward/CUDA/Reactant 1326488266 ns 1309552971.5 ns 1.01
ViT tiny (256 x 256 x 3 x 32)/forward/CUDA/Lux 91954474 ns 88993975 ns 1.03
ViT tiny (256 x 256 x 3 x 32)/forward/CPU/Reactant 2172923706 ns 2241028351 ns 0.97
ViT tiny (256 x 256 x 3 x 32)/forward/CPU/Lux 5386117875.5 ns 5778081364 ns 0.93
ViT tiny (256 x 256 x 3 x 4)/forward/CUDA/Reactant 1343810206.5 ns 1267184417.5 ns 1.06
ViT tiny (256 x 256 x 3 x 4)/forward/CUDA/Lux 7564674.5 ns 7556327.5 ns 1.00
ViT tiny (256 x 256 x 3 x 4)/forward/CPU/Reactant 1467526138.5 ns 1485636377.5 ns 0.99
ViT tiny (256 x 256 x 3 x 4)/forward/CPU/Lux 1579589341 ns 1618719051 ns 0.98
ViT tiny (256 x 256 x 3 x 16)/forward/CUDA/Reactant 1305271383.5 ns 1277921381 ns 1.02
ViT tiny (256 x 256 x 3 x 16)/forward/CUDA/Lux 11605366 ns 11579756 ns 1.00
ViT tiny (256 x 256 x 3 x 16)/forward/CPU/Reactant 1751549749.5 ns 1771132012 ns 0.99
ViT tiny (256 x 256 x 3 x 16)/forward/CPU/Lux 2633901708.5 ns 2573621234.5 ns 1.02
ViT small (256 x 256 x 3 x 16)/forward/CUDA/Reactant 1304055953.5 ns 1308609555 ns 1.00
ViT small (256 x 256 x 3 x 16)/forward/CUDA/Lux 89974859 ns 86522099 ns 1.04
ViT small (256 x 256 x 3 x 16)/forward/CPU/Reactant 2204867166 ns 2232948717 ns 0.99
ViT small (256 x 256 x 3 x 16)/forward/CPU/Lux 3598317281 ns 4522528342 ns 0.80
ViT small (256 x 256 x 3 x 32)/forward/CUDA/Reactant 1322428054.5 ns 1299910120.5 ns 1.02
ViT small (256 x 256 x 3 x 32)/forward/CUDA/Lux 122634765 ns 107942560 ns 1.14
ViT small (256 x 256 x 3 x 32)/forward/CPU/Reactant 3087499412 ns 3089121407 ns 1.00
ViT small (256 x 256 x 3 x 32)/forward/CPU/Lux 10528143013 ns 12146634131 ns 0.87
ViT base (256 x 256 x 3 x 16)/forward/CUDA/Reactant 1287379469 ns 1317150051 ns 0.98
ViT base (256 x 256 x 3 x 16)/forward/CUDA/Lux 128362123 ns 121564537 ns 1.06
ViT base (256 x 256 x 3 x 16)/forward/CPU/Reactant 3212420542 ns 3268410602 ns 0.98
ViT base (256 x 256 x 3 x 16)/forward/CPU/Lux 10296045155 ns 10888829737 ns 0.95
ViT base (256 x 256 x 3 x 4)/forward/CUDA/Reactant 1353433771.5 ns 1345840157 ns 1.01
ViT base (256 x 256 x 3 x 4)/forward/CUDA/Lux 87599937.5 ns 78626182 ns 1.11
ViT base (256 x 256 x 3 x 4)/forward/CPU/Reactant 1885686384.5 ns 2037797510.5 ns 0.93
ViT base (256 x 256 x 3 x 4)/forward/CPU/Lux 2450179426 ns 2617867593 ns 0.94

This comment was automatically generated by workflow using github-action-benchmark.

@mofeing mofeing changed the title Remove Val constraint on Base._cat signature Generalize Base._cat to non-Val, typed Base._cat_t implementation Oct 4, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mofeing mofeing marked this pull request as ready for review October 4, 2024 21:53
@mofeing
Copy link
Collaborator Author

mofeing commented Oct 4, 2024

I was thinking to add some tests, but I'll wait to #161 to land.

@jofrevalles this should fix your problem. can you test it please?

Copy link
Member

@wsmoses wsmoses left a comment

Choose a reason for hiding this comment

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

Maybe add a test?

@mofeing
Copy link
Collaborator Author

mofeing commented Oct 5, 2024

Maybe add a test?

I found that 0-dim arrays have a similar behavior to numbers when using *cat methods, so I added some tests that should be passed by 0-dim arrays even after #161

@mofeing
Copy link
Collaborator Author

mofeing commented Oct 5, 2024

@avik-pal I don't know why the shapes are wrong. It seems like it's always doing a concatenation of the first direction... but did some print debugging and shape is correct in _cat_t.

It's failing on hcat and hvcat.

@avik-pal
Copy link
Collaborator

avik-pal commented Oct 5, 2024

julia> x = fill(true)
0-dimensional Array{Bool, 0}:
1

julia> x_concrete = Reactant.to_rarray(x)
0-dimensional Array{Bool, 0}:
1

I don't think it is hitting the dispatches at all

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mofeing
Copy link
Collaborator Author

mofeing commented Oct 5, 2024

julia> x = fill(true)
0-dimensional Array{Bool, 0}:
1

julia> x_concrete = Reactant.to_rarray(x)
0-dimensional Array{Bool, 0}:
1

I don't think it is hitting the dispatches at all

Fixed! But probably you want to change this Union at this line

if mode == ArrayToConcrete && eltype(RT) <: Union{AbstractFloat, Integer}
for the PrimitiveTypes in your PR.

@avik-pal
Copy link
Collaborator

avik-pal commented Oct 5, 2024

Yes that PR has the patch

@mofeing
Copy link
Collaborator Author

mofeing commented Oct 5, 2024

hcat and vcat are working now; hvcat is the only one that fails because it doesn't call _cat_t.

@mofeing mofeing requested a review from avik-pal October 6, 2024 00:15
@mofeing
Copy link
Collaborator Author

mofeing commented Oct 6, 2024

@avik-pal it's ready now

@mofeing mofeing linked an issue Oct 6, 2024 that may be closed by this pull request
@mofeing mofeing changed the title Generalize Base._cat to non-Val, typed Base._cat_t implementation Generalize Base._cat to non-Val, typed Base._cat_t and implement typed_hcat, typed_vcat, typed_hvcat, typed_hvncat Oct 6, 2024
@mofeing mofeing merged commit f2c0e8a into main Oct 6, 2024
15 of 24 checks passed
@mofeing mofeing deleted the fix/cat-tracedrarray branch October 6, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing hcat/vcat/hvcat dispatches Error with vcat
3 participants