-
Notifications
You must be signed in to change notification settings - Fork 4
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
Patch to correct bug introduced when added conditionals in PR https://github.com/brianguenter/FastDifferentiation.jl/pull/90#issue-2487795686 #93
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #89 added function bool_methods to create overloads for conditionals and ifelse. All tests pass. Now need to change derivative calculation and code generation.
Fixes #89 turned boolean_methods into a macro and called it on package load added tests for booleans and ifelse
…., that return boolean values.
Fixes #89 added tests for comparison operators and ifelse fixed bug in boolean_methods, func variable wasn't interpolated into Expr changed make_function to use === to test for variable inclusion
Fixes #89 added ternary Node constructor to handle ifelse replace iszero and isinf methods with ones that return Node objects instead of using node_value. removed special case isfinite from number_methods. Doesn't cause tests to fail but might cause problems. Didn't document why it was there in the first place. added special eval case for ifelse to number_methods macro two sparse tests fail for obscure reasons.
Fixes #89 SparseArrays.jl needs iszero(a::Node) to return a boolean value or it's matrix contructors don't work. Can't have an iszero that returns an expression so iszero is special cased to return a boolean.
…into separate constants. This will make it easier to filter these nodes out of derivative calculations.
Fixes #89 added a new function, is_identically_zero, that replaces old iszero. iszero now returns a node expression and is_identically_zero returns a boolean.
Fixes #89 added Contitionals.jl and iterator for conditional bit values.
Fixes #89 added error message so that users attempting to compute derivatives through conditionals will know this isn't yet supported.
Fixes #89 commment change explaining consequences of adding conditionals
Need to replace ifelse with my own ifelse that generates correct if...else code. Too many functions execute illegal stuff in one of the ifelse branches which causes an exception.
…the latter evaluates all its arguments even if doing so would cause an exception changed simplify_check_cache and check_cache functions to not take a cache argument added ! to the list of supported bool expressions
…t functions written.
added 2 new methods for ifelse so it can take inputs that are Real instead of just Node updated documentation
changedderivative(a::Node, index::Val{1}) in DifferentiationRules.jl to throw correct conditional error if expression is a conditional updated README and docs about new conditional feature.
…les for it that will work with FastDifferentiation added diff rule for mod2pi moved import DiffRules from DifferentiationRules.jl to FastDifferentiation.jl to make it easier to see at a glance the deps of FD renamed diadic_non_differentiable,monadic_non_differentiable to special_diadic and special_monadic
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This corrects a bug introduced in #90 (comment) when conditionals were added.
Bug manifested when computing derivative of
x^y
which has this definition in DiffRules.jl:FastDifferentiation.jl does not support the
? :
operator so this caused an exception.This rule has been update to use the new
if_else
operator which has the semantics ofif ... else ...
, i.e., only the true or false branch is executed.The builtin
ifelse
is also supported.Many functions need the new
if_else
because exceptions may be thrown in one of the branches, for example:For these cases the new
if_else
operator is appropriate: