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

Reading doc and some discussion #220

Merged
merged 11 commits into from
Sep 6, 2024
Merged

Reading doc and some discussion #220

merged 11 commits into from
Sep 6, 2024

Conversation

sunxd3
Copy link
Collaborator

@sunxd3 sunxd3 commented Aug 9, 2024

No description provided.

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Copy link
Contributor

github-actions bot commented Aug 9, 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 │  Tapir │  Zygote │ ReverseDiff │  Enzyme │
│                     String │ String │  String │      String │  String │
├────────────────────────────┼────────┼─────────┼─────────────┼─────────┤
│                   sum_1000 │  117.0 │   0.775 │        4.84 │    1.71 │
│                  _sum_1000 │    8.0 │  1300.0 │        32.9 │  0.0841 │
│               sum_sin_1000 │   3.04 │     1.6 │        10.8 │   0.979 │
│              _sum_sin_1000 │   3.35 │   294.0 │        16.3 │    1.39 │
│                   kron_sum │   78.1 │    3.44 │       200.0 │    8.14 │
│              kron_view_sum │   88.1 │    11.1 │       243.0 │    8.36 │
│      naive_map_sin_cos_exp │   4.13 │ missing │        8.63 │    2.76 │
│            map_sin_cos_exp │   4.61 │    1.74 │        7.24 │    3.35 │
│      broadcast_sin_cos_exp │   4.56 │     2.6 │        1.64 │    2.78 │
│                 simple_mlp │   8.88 │    3.08 │        12.0 │    3.13 │
│                     gp_lml │   16.4 │    4.48 │     missing │ missing │
│ turing_broadcast_benchmark │   8.44 │ missing │        30.7 │ missing │
└────────────────────────────┴────────┴─────────┴─────────────┴─────────┘

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Basically happy with your proposals -- only a couple of things I disagree with.

docs/src/algorithmic_differentiation.md Outdated Show resolved Hide resolved
docs/src/algorithmic_differentiation.md Outdated Show resolved Hide resolved
@sunxd3
Copy link
Collaborator Author

sunxd3 commented Sep 4, 2024

done

@willtebbutt
Copy link
Member

Regarding your questions:

should s be included in this?

Almost certainly. We should fix this. I think I re-worked the example, but must have forgotten to update this bit.

these are not quite obvious to me

Could you expand one which bit is not clear?

any update to this?

Not yet, although I really need to push it forward. I think this is also how JAX thinks about these kinds of objects, so I might take a closer look at what they do.

maybe mention this is for demonstration, in the sense that Tapir will generate this automatically? (if I understand right)

Great point. Would you mind adding something to this effect?

@sunxd3
Copy link
Collaborator Author

sunxd3 commented Sep 4, 2024

sure, will update these slightly later

@sunxd3 sunxd3 marked this pull request as ready for review September 4, 2024 11:11
@willtebbutt
Copy link
Member

willtebbutt commented Sep 5, 2024

Thanks for the updated items -- I think we're nearly there!

Co-authored-by: Will Tebbutt <wct23@cam.ac.uk>
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

This is a really helpful PR, thanks! Please feel free to merge - no need to tag a release since it just modifies the docs.

@sunxd3 sunxd3 merged commit 4b84046 into main Sep 6, 2024
18 checks passed
@sunxd3 sunxd3 deleted the sunxd3-patch-1 branch September 6, 2024 09:09
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.

2 participants