JuliaDiff / FiniteDifferences.jl

High accuracy derivatives, estimated via numerical finite differences (formerly FDM.jl)
MIT License
298 stars 25 forks source link

Should `rand_tangent(2:3)` return `DoesNotExist()`? #149

Closed mzgubic closed 3 years ago

mzgubic commented 3 years ago

I can't think of any use-case where the current

julia> ChainRulesTestUtils.rand_tangent(2:3)
Composite{UnitRange{Int64}}(start = ChainRulesCore.DoesNotExist(), stop = ChainRulesCore.DoesNotExist())

would be the better option, can anyone else?

There are potentially other indices that could be updated, such as CartesianIndex and (:).

willtebbutt commented 3 years ago

Is this causing problems somewhere in particular? This is one of those things that I would have imagined ought to be fine 🤷

mzgubic commented 3 years ago

When testing rules it is easy to forget that ⊢ needs to be used for non-differentiable arguments. This would just make it work.

First time users might not know this, and might be stumped by the wall of text coming from the error (it's hard to throw an informative error). Would just make this package easier to use I suppose.

willtebbutt commented 3 years ago

Sorry, could you run me through an example of how things break if this is generated from tangent, as opposed to a DoesNotExist? I would have thought that between the to_vec machinery and the other testing machinery, this would just work as-is. i.e. when you call to_vec on this, you should get an empty vector, and when you un-vec the empty vector, you just get this thing back.

mzgubic commented 3 years ago

Oh, sure, I see what you mean. Consider:

julia> test_rrule(partialsort, rand(10), 2:3 ⊢ nothing) # works

julia> test_rrule(partialsort, rand(10), 2:3)
test_rrule: partialsort at ([0.4374488448536442, 0.09839304438482888, 0.9204000542997595, 0.852788591305361, 0.056796164155257234, 0.3343958030716154, 0.879849544196073, 0.8427410238494664, 0.27349334025715244, 0.7934843372024454], 2:3): Error During Test at /Users/mzgubic/JuliaEnvs/ChainRules.jl/dev/ChainRulesTestUtils/src/testers.jl:168
  Got exception outside of a @test
  MethodError: Cannot `convert` an object of type Vector{Float64} to an object of type UnitRange{Int64}
  Closest candidates are:
    convert(::Type{T}, ::AbstractRange) where T<:AbstractRange at range.jl:148
    convert(::Type{T}, ::LinearAlgebra.Factorization) where T<:AbstractArray at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/factorization.jl:58
    convert(::Type{T}, ::T) where T<:AbstractArray at abstractarray.jl:14
  Stacktrace:
    [1] oftype(x::UnitRange{Int64}, y::Vector{Float64})
      @ Base ./essentials.jl:375
    [2] (::FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}})(x_vec::Vector{Float64})
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/to_vec.jl:49
    [3] (::FiniteDifferences.var"#44#46"{Vector{Float64}})(x_back::FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}, l::Int64, s::Int64)
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/to_vec.jl:153
    [4] map (repeats 2 times)
      @ ./tuple.jl:252 [inlined]
    [5] (::FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}})(v::Vector{Float64})
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/to_vec.jl:152
    [6] (::ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}})(x::Vector{Float64})
      @ Base ./operators.jl:938
    [7] (::FiniteDifferences.var"#55#57"{Int64, ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}}, Vector{Float64}})(ε::Float64)
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/grad.jl:19
    [8] JuliaDiff/ChainRulesTestUtils.jl#2
      @ ./broadcast.jl:314 [inlined]
    [9] macro expansion
      @ ~/.julia/packages/StaticArrays/rdb0l/src/broadcast.jl:125 [inlined]
   [10] _broadcast
      @ ~/.julia/packages/StaticArrays/rdb0l/src/broadcast.jl:99 [inlined]
   [11] copy
      @ ~/.julia/packages/StaticArrays/rdb0l/src/broadcast.jl:26 [inlined]
   [12] materialize
      @ ./broadcast.jl:883 [inlined]
   [13] _eval_function(m::FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}, f::FiniteDifferences.var"#55#57"{Int64, ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}}, Vector{Float64}}, x::Float64, step::Float64)
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/methods.jl:249
   [14] _estimate_magnitudes(m::FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}, f::FiniteDifferences.var"#55#57"{Int64, ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}}, Vector{Float64}}, x::Float64)
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/methods.jl:378
   [15] estimate_step(m::FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, f::FiniteDifferences.var"#55#57"{Int64, ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}}, Vector{Float64}}, x::Float64)
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/methods.jl:365
   [16] (::FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}})(f::FiniteDifferences.var"#55#57"{Int64, ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}}, Vector{Float64}}, x::Float64)
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/methods.jl:193
   [17] (::FiniteDifferences.var"#54#56"{FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}}, Vector{Float64}})(n::Int64)
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/grad.jl:16
   [18] iterate
      @ ./generator.jl:47 [inlined]
   [19] _collect
      @ ./array.jl:691 [inlined]
   [20] collect_similar(cont::Base.OneTo{Int64}, itr::Base.Generator{Base.OneTo{Int64}, FiniteDifferences.var"#54#56"{FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, ComposedFunction{ComposedFunction{ComposedFunction{typeof(first), typeof(FiniteDifferences.to_vec)}, FiniteDifferences.var"#72#73"{ChainRulesTestUtils.var"#fnew#29"{ChainRulesTestUtils.var"#35#36"{NamedTuple{(), Tuple{}}, typeof(partialsort)}, Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Bool, Bool}}}}, FiniteDifferences.var"#Tuple_from_vec#45"{Tuple{Int64, Int64}, Tuple{Int64, Int64}, Tuple{typeof(identity), FiniteDifferences.var"#Vector_from_vec#32"{UnitRange{Int64}, Vector{FiniteDifferences.var"#Real_from_vec#22"}, Vector{Vector{Int64}}}}}}, Vector{Float64}}})
   [21] map(f::Function, A::Base.OneTo{Int64})
      @ Base ./abstractarray.jl:2294
   [22] #jacobian#53
      @ ~/.julia/packages/FiniteDifferences/QL8oH/src/grad.jl:15 [inlined]
   [23] jacobian
      @ ~/.julia/packages/FiniteDifferences/QL8oH/src/grad.jl:10 [inlined]
   [24] _j′vp(fdm::FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, f::Function, ȳ::Vector{Float64}, x::Vector{Float64})
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/grad.jl:80
   [25] j′vp(fdm::FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, f::Function, ȳ::Vector{Float64}, x::Tuple{Vector{Float64}, UnitRange{Int64}})
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/grad.jl:73
   [26] j′vp(::FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, ::Function, ::Vector{Float64}, ::Vector{Float64}, ::UnitRange{Int64})
      @ FiniteDifferences ~/.julia/packages/FiniteDifferences/QL8oH/src/grad.jl:76
   [27] _make_j′vp_call(fdm::FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, f::Function, ȳ::Vector{Float64}, xs::Tuple{Vector{Float64}, UnitRange{Int64}}, ignores::Tuple{Bool, Bool})
      @ ChainRulesTestUtils ~/JuliaEnvs/ChainRules.jl/dev/ChainRulesTestUtils/src/finite_difference_calls.jl:52
   [28] macro expansion
      @ ~/JuliaEnvs/ChainRules.jl/dev/ChainRulesTestUtils/src/testers.jl:204 [inlined]
   [29] macro expansion
      @ /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inlined]
   [30] test_rrule(::typeof(partialsort), ::Vector{Float64}, ::Vararg{Any, N} where N; output_tangent::ChainRulesTestUtils.Auto, fdm::FiniteDifferences.AdaptedFiniteDifferenceMethod{5, 1, FiniteDifferences.UnadaptedFiniteDifferenceMethod{7, 5}}, check_inferred::Bool, fkwargs::NamedTuple{(), Tuple{}}, rtol::Float64, atol::Float64, kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
      @ ChainRulesTestUtils ~/JuliaEnvs/ChainRules.jl/dev/ChainRulesTestUtils/src/testers.jl:169
   [31] test_rrule(::Function, ::Vector{Float64}, ::Vararg{Any, N} where N)
      @ ChainRulesTestUtils ~/JuliaEnvs/ChainRules.jl/dev/ChainRulesTestUtils/src/testers.jl:166
   [32] top-level scope
      @ REPL[32]:1
   [33] eval
      @ ./boot.jl:360 [inlined]
   [34] eval_user_input(ast::Any, backend::REPL.REPLBackend)
      @ REPL /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:139
   [35] repl_backend_loop(backend::REPL.REPLBackend)
      @ REPL /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:200
   [36] start_repl_backend(backend::REPL.REPLBackend, consumer::Any)
      @ REPL /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:185
   [37] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool)
      @ REPL /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:317
   [38] run_repl(repl::REPL.AbstractREPL, consumer::Any)
      @ REPL /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:305
   [39] (::Base.var"#874#876"{Bool, Bool, Bool})(REPL::Module)
      @ Base ./client.jl:387
   [40] #invokelatest#2
      @ ./essentials.jl:708 [inlined]
   [41] invokelatest
      @ ./essentials.jl:706 [inlined]
   [42] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool)
      @ Base ./client.jl:372
   [43] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:302
   [44] _start()
      @ Base ./client.jl:485
willtebbutt commented 3 years ago

Sorry, could you provide the whole stack trace? I would like to know why it's trying to convert a UnitRange to a Vector{Float64} in the first place 😂 . I'd bet this is yet another instance of the whole abstract types == bad, concrete types == good thing.

mzgubic commented 3 years ago

Oh yeah, sure, I've left it out since it is a monster

willtebbutt commented 3 years ago

It's hitting this rule for AbstractVectors :table_flip:

willtebbutt commented 3 years ago

Coming back to this: I feel strongly that patching this by making this a DoesNotExist is a bad idea, and that we ought to make to_vec work properly for subtypes of AbstractArray.

This is very much not a question of whether or not there is a good use-case -- rather it's about consistency.

mzgubic commented 3 years ago

Is it possible to write to_vec for a UnitRange/StepRange/StepRangeLen? I mean, it's easy to turn it into a vector, but once you perturb the vector it is no longer possible to construct a range from it, right?

In other words these various ranges seem to be outside of the domain of the to_vec function. Perhaps the fallback to_vec methods for AbstractArray/Vector should instead be StridedArray/Vector?

willtebbutt commented 3 years ago

Is it possible to write to_vec for a UnitRange/StepRange/StepRangeLen? I mean, it's easy to turn it into a vector, but once you perturb the vector it is no longer possible to construct a range from it, right?

So this is another example of the "is it an array, or is it a struct?" question. In this case I think it's clear cut that they ought to be treated as structs. If you take that view, there's really no problem -- they're just structs and we utilise the recursive definition of to_vec like we would for any other struct.

In other words these various ranges seem to be outside of the domain of the to_vec function. Perhaps the fallback to_vec methods for AbstractArray/Vector should instead be StridedArray/Vector?

I think that we should try to implement these tighter typing constraints anyway because, as we can see in this example, to_vec(::AbstractArray) doesn't work.

oxinabox commented 3 years ago

rand_tangent(2:3) should return DoesNotExist(). particularly since rand_tangent(1) returns that. We treat integers as non-pertubable. thus AbstractArray{<:Interger} should also be non-perturable.

willtebbutt commented 3 years ago

Agreed -- either that or a Composite with DoesNotExist()s in.

mzgubic commented 3 years ago

Agreed -- either that or a Composite with DoesNotExist()s in.

This is what we have now