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

Float32 Testing #414

Merged
merged 29 commits into from
Dec 11, 2024
Merged

Float32 Testing #414

merged 29 commits into from
Dec 11, 2024

Conversation

willtebbutt
Copy link
Member

@willtebbutt willtebbutt commented Dec 10, 2024

Our current approach to testing with Float32 is non-existent -- we just assume that all rules work correctly for both provided that we've tested on Float64. This is probably fine (albeit somewhat scary) when the only difference between the F64 and F32 rule is a type parameter, as has been the case so far. I will shortly be adding a lot of GPU rules, for which Float64 rules might not be provided. Consequently, it is about to become important that we have the ability to properly test the correctness of rules involving Float32s.

This PR adds this. The existing functionality is basically fine, it's looking like I just need to

  1. use a range of step sizes for finite differences, and ask that AD gives an answer which is close to at least one of them,
  2. upgrade from forward differences to central differences (I had forgotten I was using forward rather than central -- we should probably always have been using central)

I'm now going through my test suite, adding for-loops in various places to ensure that both Float64 and Float32 are tested.

update: this is nearly there now. A few bugs (not numerical correctness issues happily) were uncovered as I went through and started to add in Float32 tests everywhere. It looks like a memory-related bug has been revealed in the LAPACK rules, so I'll need to resolve that before merging.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 99.60938% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/rrules/lapack.jl 99.42% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/rrules/blas.jl 99.18% <100.00%> (+0.01%) ⬆️
src/rrules/builtins.jl 97.93% <ø> (ø)
src/rrules/fastmath.jl 100.00% <100.00%> (ø)
src/rrules/linear_algebra.jl 100.00% <100.00%> (ø)
src/rrules/low_level_maths.jl 100.00% <100.00%> (ø)
src/test_utils.jl 93.02% <100.00%> (+0.06%) ⬆️
src/utils.jl 83.78% <100.00%> (+0.45%) ⬆️
src/rrules/lapack.jl 99.43% <99.42%> (+7.49%) ⬆️

Copy link
Contributor

github-actions bot commented Dec 10, 2024

Performance Ratio:
Ratio of time to compute gradient and time to compute function.
Warning: results are very approximate! See here for more context.

┌────────────────────────────┬──────────┬─────────┬─────────────┬─────────┐
│                      Label │ Mooncake │  Zygote │ ReverseDiff │  Enzyme │
│                     String │   String │  String │      String │  String │
├────────────────────────────┼──────────┼─────────┼─────────────┼─────────┤
│                   sum_1000 │     71.7 │     1.1 │        5.61 │ missing │
│                  _sum_1000 │     6.65 │  1340.0 │        33.5 │ missing │
│               sum_sin_1000 │     2.28 │    1.74 │        10.7 │ missing │
│              _sum_sin_1000 │     2.73 │   259.0 │        13.2 │ missing │
│                   kron_sum │     73.7 │    4.68 │       239.0 │ missing │
│              kron_view_sum │     64.7 │    10.4 │       264.0 │ missing │
│      naive_map_sin_cos_exp │     2.54 │ missing │        7.16 │ missing │
│            map_sin_cos_exp │     2.74 │    1.52 │        6.96 │ missing │
│      broadcast_sin_cos_exp │      2.6 │    2.23 │        1.46 │ missing │
│                 simple_mlp │     3.55 │    2.48 │         4.9 │ missing │
│                     gp_lml │     4.68 │    1.51 │     missing │ missing │
│ turing_broadcast_benchmark │     3.39 │ missing │        31.4 │ missing │
│         large_single_block │     4.13 │  4070.0 │        30.7 │ missing │
└────────────────────────────┴──────────┴─────────┴─────────────┴─────────┘

@willtebbutt willtebbutt reopened this Dec 11, 2024
@willtebbutt willtebbutt merged commit 102f900 into main Dec 11, 2024
72 checks passed
@willtebbutt willtebbutt deleted the wct/f32 branch December 11, 2024 13:10
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.

1 participant