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

More problems regarding round #164

Closed
SouthEndMusic opened this issue Aug 13, 2024 · 13 comments · Fixed by #168
Closed

More problems regarding round #164

SouthEndMusic opened this issue Aug 13, 2024 · 13 comments · Fixed by #168

Comments

@SouthEndMusic
Copy link
Contributor

SouthEndMusic commented Aug 13, 2024

I did a quick test with the current main and I still have a problem with the function round where I want the return type to be an integer (and thus not depend smoothly on the input). f(x) = Int(round(x)) doesn't work:

MethodError: no method matching Int64(::SparseConnectivityTracer.Dual{Float64, SparseConnectivityTracer.GradientTracer{…}})

Is that something you plan to support? Or should I resolve this in a different way? Still in the context of DataInterpolations by the way.

@SouthEndMusic
Copy link
Contributor Author

This is the same problem as mentioned in #152 (comment)

@adrhill
Copy link
Owner

adrhill commented Aug 13, 2024

I'll take a look! Could you check whether round(Int, x) works?

@adrhill
Copy link
Owner

adrhill commented Aug 13, 2024

The problem in #152 is now part of our test suite, so I'd be surprised if it doesn't work:

@test J(x -> round(Int, x), 1.1) [0;;]
@test J(x -> round(Bool, x), 1.1) [0;;]

@adrhill
Copy link
Owner

adrhill commented Aug 13, 2024

If you want an explicit conversation for some reason, convert(Int, round(x)) should work as well.

EDIT: Should work with TracerLocalSparsityDetector after #166 is merged, sorry about that!

@SouthEndMusic
Copy link
Contributor Author

Thanks. I'm running the unreleased master of DataInterpolations In which there are changes to the relevant code.

@SouthEndMusic
Copy link
Contributor Author

The problem seems to be that SparseConnectivityTracer.Dual{Int} <: Integer doesn't hold, but that's probably by design

@SouthEndMusic
Copy link
Contributor Author

Why is the instance of SparseConnectivityTracer.Dual{Int} created in the first place? Doesn't that indicate no connectivity anyway? Can't it just be an Int?

@adrhill
Copy link
Owner

adrhill commented Aug 13, 2024

The problem seems to be that SparseConnectivityTracer.Dual{Int} <: Integer doesn't hold, but that's probably by design

If this was possible, it would indeed simplify things a lot. Unfortunately, Julia's type system doesn't allow it.
Dual numbers are Reals and a thin wrapper around a primal number (in your case an Int) and a tracer which does the bookkeeping for the sparsity pattern:

struct Dual{P<:Real,T<:AbstractTracer} <: Real
primal::P
tracer::T

Why is the instance of SparseConnectivityTracer.Dual{Int} created in the first place? Doesn't that indicate no connectivity anyway? Can't it just be an Int?

In this specific instance, we could (and I guess should?) indeed return an Int, since round gets rid of all first- and second-order derivative information.
EDIT: opened #167 to track.

More generally speaking, for things like Int(mydual) or convert(Int, mydual), we are forced to return a Dual instead of an Int. This isn't optimal, since returning unexpected types leads to method invalidations and therefore increased compile times. But if we didn't do this, these type conversions would strip all derivative information by essentially removing the tracer part of the Dual.

This is somewhat of a design dilemma for us: do we (a) make the code work on most user-provided functions at the cost of invalidating some code or do we (b) throw a lot of errors and require users to rewrite their code such that it is compatible with SCT? For now, we aim for (a).
Out of curiosity, I took a look at ForwardDiff's approach to Int(x). It is more conservative and throws an InexactError if its derivative information is nonzero. Only if it is zero, it returns the primal value as an Int:

julia> using ForwardDiff

julia> ForwardDiff.derivative(x -> convert(Int, x), 1)
ERROR: InexactError: Int(Int64, Dual{ForwardDiff.Tag{var"#3#4", Int64}}(1,1))
Stacktrace:
 [1] Int64
   @ ~/.julia/packages/ForwardDiff/PcZ48/src/dual.jl:364 [inlined]
 [2] convert
   @ ./number.jl:7 [inlined]
 [3] #3
   @ ./REPL[3]:1 [inlined]
 [4] derivative(f::var"#3#4", x::Int64)
   @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/derivative.jl:14
 [5] top-level scope
   @ REPL[3]:1

Whether we stick with approach (a) and the invalidations or whether we move to approach (b) is something we should revisit before we tag a v1.0.0 release. CC @gdalle.

@gdalle
Copy link
Collaborator

gdalle commented Aug 14, 2024

More generally speaking, for things like Int(mydual) or convert(Int, mydual), we are forced to return a Dual instead of an Int. This isn't optimal, since returning unexpected types leads to method invalidations and therefore increased compile times. But if we didn't do this, these type conversions would strip all derivative information by essentially removing the tracer part of the Dual.

That's not true for Int, as discussed in #167, because Int conversion obliterates derivatives. However it is true e.g. for convert(Float64, mydual).

@adrhill
Copy link
Owner

adrhill commented Aug 14, 2024

Let's discuss this in #167!

@adrhill
Copy link
Owner

adrhill commented Aug 15, 2024

To come back to the initial issue @SouthEndMusic:
I'll add a conservative Int(x::Dual) overload à la ForwardDiff and return an Int iff the tracer is empty.

@gdalle
Copy link
Collaborator

gdalle commented Aug 15, 2024

Wait why? We can return an int in every case. And what you suggest sounds type unstable.

@adrhill
Copy link
Owner

adrhill commented Aug 15, 2024

Once again, let's discuss this in #167, it deserves its own issue outside of one on round.

ForwardDiff's approach I linked above is not type unstable: an InexactError is thrown when the derivative information would be lost.

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 a pull request may close this issue.

3 participants