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 support for ComponentVector #145

Closed
SouthEndMusic opened this issue Aug 6, 2024 · 12 comments · Fixed by #146
Closed

Add support for ComponentVector #145

SouthEndMusic opened this issue Aug 6, 2024 · 12 comments · Fixed by #146
Labels
input tracing Issue regarding creation of tracers from input.

Comments

@SouthEndMusic
Copy link
Contributor

Currently tracing through a function with ComponentVector input and output does not work. E.g.

using SparseConnectivityTracer: TracerSparsityDetector, jacobian_sparsity
using ComponentArrays: ComponentVector
detector = TracerSparsityDetector()

function f!(output, input)
    output.x .= input.y
    output.y .= input.x
end

input = ComponentVector{Float64}(; x=rand(5), y=rand(5))
output = zero(input)

jacobian_sparsity(f!, output, input, detector)

yields

type Array has no field y
@SouthEndMusic SouthEndMusic changed the title Add support for ComponentVectors Add support for ComponentVector Aug 6, 2024
@gdalle gdalle linked a pull request Aug 6, 2024 that will close this issue
@gdalle
Copy link
Collaborator

gdalle commented Aug 6, 2024

Thanks for reporting, this should be solved by #146! Can you try out the branch from #146 on your real use case, see if other problems pop up?

@SouthEndMusic
Copy link
Contributor Author

I gave it a try, and now it breaks down on DataInterpolations, specifically https://github.com/SciML/DataInterpolations.jl/blob/35db42ea22fae1caeb92ca6cfbb4a204ace4e401/src/interpolation_methods.jl#L2. You document that you do not support <, >, but I feel like this should work as it is a 'dead end' of the trace that can be ignored

@gdalle
Copy link
Collaborator

gdalle commented Aug 6, 2024

What do you mean by dead end that can be ignored?

@gdalle
Copy link
Collaborator

gdalle commented Aug 6, 2024

Note that SparseConnectivityTracer errors at the comparison. It has no way to know that an error is thrown right after

@SouthEndMusic
Copy link
Contributor Author

The jacobian sparsity from symbolics doesn't error on this, so I wonder what is going on there.

I can imagine that the tracing goes wrong if the path trough the code branches out based on the input in a way that affects the output. But that is not what happens here

@gdalle
Copy link
Collaborator

gdalle commented Aug 6, 2024

Yeah the Symbolics approach works in a different way, and can more easily handle conditionals than our operator overloading method.
The reason it fails is that TracerSparsityDetector aims to be independent of the input value, which is why we can't return anything meaningful for x < y (we don't know if it's true or false).
As a workaround, if you're 100% sure that the sparsity pattern does not depend on the input value, you can use TracerLocalSparsityDetector. But we will probably have to add custom overloads for DataInterpolations anyway, as noted in #74 (comment)

@SouthEndMusic
Copy link
Contributor Author

SouthEndMusic commented Aug 6, 2024

With TracerLocalSparsityDetector it fails with

ERROR: MethodError: isless(::SparseConnectivityTracer.Dual{Float64, SparseConnectivityTracer.GradientTracer{SparseConnectivityTracer.IndexSetGradientPattern{Int64, BitSet}}}, ::Float64) is ambiguous.

Candidates:
  isless(dx::D, y::Real) where D<:SparseConnectivityTracer.Dual
    @ SparseConnectivityTracer path\to\.julia\packages\SparseConnectivityTracer\rsu1Y\src\overloads\dual.jl:24
  isless(x::Real, y::AbstractFloat)
    @ Base operators.jl:178

Possible fix, define
  isless(::D, ::AbstractFloat) where D<:SparseConnectivityTracer.Dual

@gdalle
Copy link
Collaborator

gdalle commented Aug 6, 2024

okay that one is an easy fix @adrhill, we just have to mimic the definitions in Base and do

isless(::Dual, ::AbstractFloat)
isless(::AbstractFloat, ::Dual)

@adrhill adrhill mentioned this issue Aug 6, 2024
@adrhill
Copy link
Owner

adrhill commented Aug 6, 2024

Indeed, it's an easy fix.

@adrhill adrhill added the input tracing Issue regarding creation of tracers from input. label Aug 7, 2024
@adrhill
Copy link
Owner

adrhill commented Aug 7, 2024

@SouthEndMusic The ComponentVector and isless issues should be fixed on the main branch. Please let us know if something else pops up! Usually it's just a question of missing overloads, which are quickly added. :)

@SouthEndMusic
Copy link
Contributor Author

SouthEndMusic commented Aug 7, 2024

Thanks for the work thus far. I now get exactly the same error on isless, triggered at a different place:

https://github.com/SciML/FindFirstFunctions.jl/blob/3c67f565c321a53a9798ab0598ad916ecd1820f7/src/FindFirstFunctions.jl#L179

This is also triggered via DataInterpolations.

@adrhill
Copy link
Owner

adrhill commented Aug 7, 2024

Does the error appear on isless or Base.Order.lt(o, x, v[lo])? Do you mind opening a new issue with the stack trace?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
input tracing Issue regarding creation of tracers from input.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants