-
Notifications
You must be signed in to change notification settings - Fork 1
Generalise Broadcasts using ChainRules.jl #17
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
=======================================
Coverage ? 82.14%
=======================================
Files ? 5
Lines ? 112
Branches ? 0
=======================================
Hits ? 92
Misses ? 20
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/arithmetic.jl
Outdated
| end | ||
|
|
||
| # Define a lookup table typeof(function) => function | ||
| function_lookup = Dict{DataType, Function}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a standard pattern? I haven't seen this before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, I wasn't able to find another way in Julia to get the function from the typeof(function) provided in the frule signature. I've seen/used a similar pattern in various LeetCode problems for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a citation as a comment to where you saw this pattern?
I'm wary of using obscure patterns. I would look at the packages that depend on ChainRules.jl to see if there's an established pattern. But if you can't find anything, it's fine: I tend to use a "test-driven development" point of view the unit tests are more important than the details of the implementation (which can be modified freely as long as tests pass).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another look at the implementation and realised the lookup table was unnecessary
Co-authored-by: Sheehan Olver <[email protected]>
This pull request adds generalised broadcasting for DualVectors based on ChainRules.jl for:
As well as this, some other changes have been made:
vcat