Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

agerlach
Copy link

Closes #564

@agerlach
Copy link
Author

@Kolaru Based on your comment in the issue, do you think DiffRules is light enough as a dep?

@lbenet
Copy link
Member

lbenet commented Jun 14, 2023

Looks ok, though i think we should also include the case for DecoratedIntervals, which at the moment is not too clear to me...

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4404658) 90.46% compared to head (12d5ca5) 90.49%.
Report is 237 commits behind head on master.

❗ Current head 12d5ca5 differs from pull request most recent head 93562b8. Consider uploading reports for the commit 93562b8 to get more accurate results

❗ 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              
Files Coverage Δ
src/IntervalArithmetic.jl 100.00% <ø> (ø)
src/ad.jl 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agerlach
Copy link
Author

I have no experience with DecoratedIntervals and I'm not even clear what they actually are from the docs. I would need some guidance on that.

@lbenet
Copy link
Member

lbenet commented Jun 15, 2023

I have no experience with DecoratedIntervals and I'm not even clear what they actually are from the docs. I would need some guidance on that.

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 sqrt(-1 .. 1).

Going back to _abs_deriv, I guess that if the interval has a definite sign including perhaps zero, the decoration would be unchanged; if the two signs are involved I guess the decoration would be reduced...

But maybe we should leave this for another PR.

@lbenet
Copy link
Member

lbenet commented Jun 15, 2023

One more suggestion/question: Could we have (either through Requires or by exploiting the new pkg extensions (weakdeps) for julia 1.9), that DiffEqRules is optional?

@agerlach
Copy link
Author

My personal opinion is that this does not warrant doing an extension. DiffRules is a light weight dependency. If you want to do an extension and support <1.9 you may need to add an additional deps to be backwards compatible with Requires.jl anyway.

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.

@agerlach
Copy link
Author

agerlach commented Jul 6, 2023

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 DiffRules I don't think an extension is warranted.

@Kolaru Kolaru self-assigned this Jul 6, 2023
@Kolaru Kolaru self-requested a review July 6, 2023 12:46
Copy link
Collaborator

@Kolaru Kolaru left a 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.

src/ad.jl Outdated Show resolved Hide resolved
src/ad.jl Show resolved Hide resolved
test/interval_tests/ad.jl Outdated Show resolved Hide resolved
agerlach and others added 4 commits October 12, 2023 08:52
Co-authored-by: Benoît Richard <kolaru@hotmail.com>
Co-authored-by: Benoît Richard <kolaru@hotmail.com>
@OlivierHnt
Copy link
Member

Since the derivative is not defined at 0, the decoration should probably decay.
Also, for bare intervals, I am not sure what should be returned when the interval contains 0. Perhaps the empty interval?

@agerlach
Copy link
Author

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 DecoratedInterval, so I'm unclear how that changes anything.

@agerlach
Copy link
Author

@OlivierHnt For additionally context, I had this linked in #564, JuliaDiff/DiffRules.jl#98

@agerlach
Copy link
Author

Also, the tests are now failing after merging from master. I'm not quite sure why though.

@OlivierHnt
Copy link
Member

OlivierHnt commented Oct 12, 2023

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.
In our case here, the derivative of abs is undefined at zero. We can return a bounded, non-empty interval if that is meaningful in AD as you suggest; at the same time, the decoration allows us to indicate that something spooky (mathematically) went on, typically we would tag the output interval with "trv" (as in "trivial") to let someone interested in computer-assisted proofs that they should double (or triple) check everything.

We cannot do this for a bare interval (which is currently represented by the structure Interval). In this case it probably makes more sense to return an empty interval.

@agerlach
Copy link
Author

agerlach commented Oct 12, 2023

I see. It seems that the ultimate question is whether or not derivative should return the strict derivative, or a sub differential. In the later, I believe this meets the definition. https://en.wikipedia.org/wiki/Subderivative#Definition.

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.

@OlivierHnt
Copy link
Member

Superseded by PR #593.

@OlivierHnt OlivierHnt closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Derivative of abs(::Interval)
5 participants