Skip to content

Commit

Permalink
make_function returns wrong result with sparse Jacobian for very si…
Browse files Browse the repository at this point in the history
…mple functions only

Fixes #67

added NoOp Node type and wrapped all roots in NoOp to ensure that nodes used in multiple roots all get edges.
  • Loading branch information
brianguenter committed Apr 10, 2024
1 parent 9245a43 commit 4953981
Showing 1 changed file with 17 additions and 1 deletion.
18 changes: 17 additions & 1 deletion src/ExpressionGraph.jl
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,22 @@ derivative(::typeof(+), args::NTuple{N,Any}, ::Val{I}) where {I,N} = Node(1)

function_variable_derivative(a::Node, index::Val{i}) where {i} = check_cache((Differential, children(a)[i]), EXPRESSION_CACHE)

"""When constructing `DerivativeGraph` with repeated values in roots, e.g.,
```julia
@variables x
f = sin(x)
gr = DerivativeGraph([f,f,f])
```
all three of the f values reference the same element. To ensure that `partial_edges` creates an edge for each one of the roots we need a `NoOp` function. The derivative of `NoOp` is 1.0; the sole purpose of this node type is to ensure that the resulting derivative graph has a separate edge for each repeated root value. There are other ways this might be accomplished but this is the simplest, since it can be performed on the original `Node` graph before the recursive `partial_edges` traversal."""
struct NoOp
end

function create_NoOp(child)
return Node(NoOp(), child)
end

derivative(NoOp, arg::Tuple{T}, ::Val{1}) where {T} = 1.0

function derivative(a::Node, index::Val{1})
# if is_variable(a)
# if arity(a) == 0
Expand Down Expand Up @@ -501,7 +517,7 @@ function postorder(roots::AbstractVector{T}) where {T<:Node}
variables = Vector{Node}(undef, 0)

for root in roots
_postorder_nodes!(root, nodes, variables, node_to_index)
_postorder_nodes!(create_NoOp(root), nodes, variables, node_to_index)
end
return node_to_index, nodes, variables
end
Expand Down

0 comments on commit 4953981

Please sign in to comment.