-
Notifications
You must be signed in to change notification settings - Fork 25
fix: improve support for empty inputs (still not guaranteed) #835
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
Conversation
I gave it a try too and it's a bit harder than it looks to make this work consistently. First, there are unavoidable discrepancies across backends:
This suggests we need to handle batch sizes of 0 while also setting a minimum of 1 when we can.
|
To give you an example of the kind of changes necessary, you can take a look at this commit. I'm not sure supporting empty arrays is worth it, especially if support is always gonna be divergent across backends as you noted in #802 |
Thanks for looking into it in more detail! I definitely see the issues with making it consistent. I didn't really set out to solve it for all backends, that was a task that was too annoying and possibly also too unimportant -- hence my very tiny single commit 😄. I don't think that DI needs to explicitly guarantee or document that empty inputs will work for all backends; but it's just nice to not get an error and if some code could be tweaked to make that happen, that's a net benefit IMO. Did that commit alone break ForwardDiff or some other backend? I was sort of hoping that CI would catch issues that I hadn't thought of or didn't know of (of which I'm sure there are plenty). But also my impression was that the lines I changed should only affect empty inputs. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
==========================================
+ Coverage 78.24% 88.12% +9.87%
==========================================
Files 128 132 +4
Lines 7805 7872 +67
==========================================
+ Hits 6107 6937 +830
+ Misses 1698 935 -763
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I don't think so but I wanted to add proper tests covering this line, and to make sure that the global approach is at least vaguely coherent. I tried defining a batch size of 0 for empty arrays by default, but I think you're right that 1 is better. I ended up adding a bunch of empty test scenarios in DIT to make my life easier. DI's default implementation of |
Enzyme tests failing due to EnzymeAD/Enzyme.jl#2523 DIT tests failing because I must have introduced a type instability somewhere |
I ran into #802 again while refactoring some tests on Bijectors.jl, so this is a patch that lets forward-mode Enzyme work with empty inputs.