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

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

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

devmotion
Copy link
Member

Reverts #98, due to the problem explained in #98 (comment).

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 #98 (comment) provides an argument for taking 1.

@codecov
Copy link

codecov bot commented Jun 3, 2023

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
@@           Coverage Diff           @@
##           master     #100   +/-   ##
=======================================
  Coverage   97.86%   97.86%           
=======================================
  Files           3        3           
  Lines         187      187           
=======================================
  Hits          183      183           
  Misses          4        4           
Impacted Files Coverage Δ
src/rules.jl 100.00% <100.00%> (ø)

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

@devmotion
Copy link
Member Author

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

@agerlach
Copy link
Contributor

agerlach commented Jun 5, 2023

@devmotion My apologies for the delay.

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

@devmotion devmotion merged commit 64145a8 into master Jun 5, 2023
@devmotion devmotion deleted the revert-98-abs branch June 5, 2023 19:47
@oxinabox
Copy link
Member

oxinabox commented Jun 6, 2023

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

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.

3 participants