search

FluxML / Zygote.jl

21st century AD
https://fluxml.ai/Zygote.jl/
Other
1.48k stars 210 forks source link

Remove `@adjoint`s for `sort` and `filter` #1493

Closed lkdvos closed 9 months ago

lkdvos commented 9 months ago

This removes the definitions of sort and filter, as these are covered by ChainRules. This also fixes #1492 , as the ChainRules rrule for sort includes all keywords.

devmotion commented 9 months ago

This also fixes #1492 , as the ChainRules rrule for sort includes all keywords.

I think it would be good to add a test for #1492.

darsnack commented 9 months ago

The Molly.jl failure seems real, but the downstream test set is complex/foreign enough that I can't decipher where sort or filter are used. @jgreener64 can you confirm that this change is causing the failures and where?

jgreener64 commented 9 months ago

That test does fail sometimes due to stochasticity, I should improve it. The Molly CI run for the latest commit seems fine though, so this change doesn't seem to break anything.

lkdvos commented 9 months ago

I think in that case this is ready for review + merge, unless I need to add something else?