search

CDCgov / Rt-without-renewal

https://cdcgov.github.io/Rt-without-renewal/
Apache License 2.0
17 stars 3 forks source link

Remove Zygote from benchmarks #364

Closed seabbs closed 3 months ago

seabbs commented 3 months ago

At the moment we include Zygote in our benchmarking but as per #339 support across our tools is very poor and performance is bad. To reduce the number of warnings we could remove Zygote for now and aim to fix in #339 (and then turn the benchmarks back on).

seabbs commented 3 months ago

the downside of this is reducing visibility

SamuelBrand1 commented 3 months ago

NB: Zygote only works with strict immutability so to get it to work well requires rrules and strictly careful attention.

seabbs commented 3 months ago

or being strict about immutability? The downside is we lose track of what doesn't work but if we don't think we can support it in any reasonable term its only a small downside

SamuelBrand1 commented 3 months ago

I think the Julia AD community is moving towards having better support for mutability (e.g. https://github.com/compintell/Tapir.jl?tab=readme-ov-file#coverage-of-more-of-the-language ) rather than recommending immutability everywhere. This suggests that Zygote is getting depreciated outside of core ML packages (e.g. Flux.jl, Lux.jl) where it is "easy" to maintain immutability between layers. YMMV though.