-
Notifications
You must be signed in to change notification settings - Fork 72
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
add AD hook for abs() per DiffRules #565
Conversation
@Kolaru Based on your comment in the issue, do you think DiffRules is light enough as a dep? |
Looks ok, though i think we should also include the case for |
Codecov ReportAll modified lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #565 +/- ##
==========================================
+ Coverage 90.46% 90.49% +0.03%
==========================================
Files 25 26 +1
Lines 1803 1809 +6
==========================================
+ Hits 1631 1637 +6
Misses 172 172
☔ View full report in Codecov by Sentry. |
I have no experience with |
I have also essentially no experience with decorations... Decorations are a label which tracks the kind of operations involved in the computation. So, e.g., you can know if during a computation you evaluated something like Going back to But maybe we should leave this for another PR. |
One more suggestion/question: Could we have (either through |
My personal opinion is that this does not warrant doing an extension. The ultimate question is how far to take the extension capabilities when lightweight "Core" deps exist. Should things like PlotRecipes and StaticArrayCore be ext? It seems like a lot of developmental and organizational overhead for what you get in return. |
Any thoughts on keeping the dep and merging vs doing an extension? I would really like to see this fix merged. As mentioned above, given the lightweight nature of |
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 am fine with having ChainRulesCore as a dependency. I think we are not yet at the point of trying to reduce TTFX as much as possible with this package.
I have few suggestions for the rest, mostly to support DecoratedInterval.
Co-authored-by: Benoît Richard <[email protected]>
Co-authored-by: Benoît Richard <[email protected]>
Since the derivative is not defined at |
It is standard in AD to take reasonable sub-derivative in these cases. For abs, 0 is the most reasonable. See https://juliadiff.org/ChainRulesCore.jl/dev/maths/nondiff_points.html. This is what occurs in this PR. I don't really understand the role of |
@OlivierHnt For additionally context, I had this linked in #564, JuliaDiff/DiffRules.jl#98 |
Also, the tests are now failing after merging from master. I'm not quite sure why though. |
I understand that it is convenient to set the derivative at zero to be zero, but I wonder about the mathematical repercussions of this choice (we want to be able to prove mathematical theorems with IntervalArithmetic.jl). The point of a decoration is to have a way of tracking what went on during the successive operations that resulted in the final output. We cannot do this for a bare interval (which is currently represented by the structure |
I see. It seems that the ultimate question is whether or not Either way, it seems reasonable that a user may want either as there are certainly many applications for IA outside of the context of theorem proving. |
Superseded by PR #593. |
Closes #564