JuliaDiff / DiffRules.jl

A simple shared suite of common derivative definitions
Other
74 stars 38 forks source link

Revert "Update abs diff rule to 0 at non-differentiable point" #100

Closed devmotion closed 1 year ago

devmotion commented 1 year ago

Reverts JuliaDiff/DiffRules.jl#98, due to the problem explained in https://github.com/JuliaDiff/DiffRules.jl/pull/98#issuecomment-1574420052.

Maybe this issue explains why the "derivative" at 0 was defined as 1 originally (first in ForwardDiff, and then in DiffRules when thw rule was moved here) - but unfortunately neither DiffRules nor ForwardDiff tests it.

In principle, it seems that one could argue for any subderivative, ie., any number in [-1, 1], as the "derivative" at 0. The abs function is also discussed in the ChainRules docs, where it is arfued for taking the subderivative 0 and against -1 or 1. However, yhe example in https://github.com/JuliaDiff/DiffRules.jl/pull/98#issuecomment-1574420052 provides an argument for taking 1.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (fee3857) 97.86% compared to head (4cc78f0) 97.86%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #100 +/- ## ======================================= Coverage 97.86% 97.86% ======================================= Files 3 3 Lines 187 187 ======================================= Hits 183 183 Misses 4 4 ``` | [Impacted Files](https://app.codecov.io/gh/JuliaDiff/DiffRules.jl/pull/100?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | Coverage Δ | | |---|---|---| | [src/rules.jl](https://app.codecov.io/gh/JuliaDiff/DiffRules.jl/pull/100?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL3J1bGVzLmps) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

devmotion commented 1 year ago

Pinging @agerlach and @oxinabox since they were involved in #98.

agerlach commented 1 year ago

@devmotion My apologies for the delay.

Given the downstream issues caused, I see no reason why not to revert #98.

oxinabox commented 1 year ago

this seems fine, would be good to fix the comment though. Since the comment there talked about working with Interval types. And the changed version worked better with interval types.

The hessian coming out at zero is just a special case of the derivative being computered as zero for

function f(x)
     if x==1.0
        1.0
     else
        x
     end
end

f'(1.0) == 0

right?

To which this is not a satisfying solution but it is better than breaking actual code, for conceptual improvements

devmotion commented 1 year ago

I guess the comment is correct but possibly should be clarified: the _abs_deriv function was intended as a hook for eg interval packages - they were supposed to implement it for interval types.

devmotion commented 1 year ago

I agree, it's not very satisfying that we run into problems when defining the derivative at 0 as 0. Maybe it's generally safer to take the left or right limit, as long as such special branches lead to zero derivatives in e.g. ForwardDiff?

devmotion commented 1 year ago

BTW I checked and the problem shows up also with ForwardDiff#master.