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

Deepcopy output rather than copy #112

Closed
wants to merge 2 commits into from
Closed

Conversation

willtebbutt
Copy link
Member

@willtebbutt willtebbutt commented Apr 2, 2024

@gdalle could you see if this branch sorts your problem out? If so, I'll merge it, as I don't think it does any harm on my end.

@gdalle
Copy link
Collaborator

gdalle commented Apr 2, 2024

It probably slows things down and makes them type unstable. Is it really a deep copy that you need? Why even copy?

Copy link
Contributor

github-actions bot commented Apr 2, 2024

Performance Ratio:

┌────────────────────────────┬────────┬─────────┬─────────────┬─────────┐
│                      Label │  Tapir │  Zygote │ ReverseDiff │  Enzyme │
│                     String │ String │  String │      String │  String │
├────────────────────────────┼────────┼─────────┼─────────────┼─────────┤
│                        sum │   98.6 │   0.357 │        2.32 │    0.54 │
│                       _sum │   25.7 │   485.0 │        26.7 │   0.121 │
│                   kron_sum │  171.0 │    3.42 │       233.0 │    20.8 │
│              kron_view_sum │  221.0 │    9.97 │       229.0 │     7.3 │
│      naive_map_sin_cos_exp │   8.62 │ missing │        8.86 │    2.77 │
│            map_sin_cos_exp │   10.7 │    1.82 │        7.56 │    3.44 │
│      broadcast_sin_cos_exp │   10.1 │    2.64 │        1.68 │    2.86 │
│                 simple_mlp │   23.0 │     3.1 │        11.3 │    3.63 │
│                     gp_lml │   42.7 │    4.16 │     missing │ missing │
│ turing_broadcast_benchmark │   19.3 │ missing │        37.9 │ missing │
└────────────────────────────┴────────┴─────────┴─────────────┴─────────┘

@gdalle
Copy link
Collaborator

gdalle commented Apr 2, 2024

See JuliaDiff/DifferentiationInterface.jl#126 for the new issue ^^

@gdalle
Copy link
Collaborator

gdalle commented Apr 2, 2024

@gdalle
Copy link
Collaborator

gdalle commented Apr 4, 2024

Probably worth closing for now? Maybe in favor of a custom _copy that handles the nothing case as suggested in JuliaDiff/DifferentiationInterface.jl#126 (comment)?
I'm usually not a fan of these manual patches, but I think it's justified by the relevance of DifferentiationInterface for testing Tapir

@willtebbutt willtebbutt closed this Apr 4, 2024
@willtebbutt willtebbutt deleted the wct/deepcopy-output branch April 4, 2024 11:00
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