tpapp / TransformVariables.jl

Transformations to contrained variables from ℝⁿ.
Other
66 stars 14 forks source link

no `zip` in `_inverse_eltype_tuple` #91

Closed chriselrod closed 2 years ago

chriselrod commented 2 years ago

The added test will fail on the current master and the latest release. On 0.6.1, I get:

julia> using TransformVariables, Test

julia> trf = as((x0 = TransformVariables.ShiftedExp{true, Float32}(0f0), x1 = TransformVariables.Identity(), x2 = UnitSimplex(7), x3 = TransformVariables.CorrCholeskyFactor(5), x4 = as(Real, -∞, 1), x5 = as(Array, 10, 2), x6 = as(Array, as𝕀, 10), x7 = as((a = asℝ₊, b = as𝕀)), x8 = TransformVariables.UnitVector(10), x9 = TransformVariables.ShiftedExp{true, Float32}(0f0), x10 = TransformVariables.ShiftedExp{true, Float32}(0f0), x11 = TransformVariables.ShiftedExp{true, Float32}(0f0), x12 = TransformVariables.ShiftedExp{true, Float32}(0f0), x13 = TransformVariables.Identity(), x14 = TransformVariables.ShiftedExp{true, Float32}(0f0), x15 = TransformVariables.ShiftedExp{true, Float32}(0f0), x16 = TransformVariables.ShiftedExp{true, Float32}(0f0), x17 = TransformVariables.ShiftedExp{true, Float64}(0.0)));

julia> vx = randn(@inferred(TransformVariables.dimension(trf)));

julia> x = TransformVariables.transform(trf, vx);

julia> @test @inferred(TransformVariables.inverse_eltype(trf, x)) === Float64
Error During Test at REPL[10]:1
  Test threw exception
  Expression: #= REPL[10]:1 =# @inferred(TransformVariables.inverse_eltype(trf, x)) === Float64
  return type Type{Float64} does not match inferred return type Any
  Stacktrace:
    [1] error(s::String)
      @ Base ./error.jl:35
    [2] top-level scope
      @ ~/Documents/languages/juliarelease/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:464
    [3] eval
      @ ./boot.jl:368 [inlined]
    [4] eval_user_input(ast::Any, backend::REPL.REPLBackend)
      @ REPL ~/Documents/languages/juliarelease/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:151
    [5] repl_backend_loop(backend::REPL.REPLBackend)
      @ REPL ~/Documents/languages/juliarelease/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:247
    [6] start_repl_backend(backend::REPL.REPLBackend, consumer::Any)
      @ REPL ~/Documents/languages/juliarelease/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:232
    [7] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool)
      @ REPL ~/Documents/languages/juliarelease/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:369
    [8] run_repl(repl::REPL.AbstractREPL, consumer::Any)
      @ REPL ~/Documents/languages/juliarelease/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:356
    [9] (::Base.var"#964#966"{Bool, Bool, Bool})(REPL::Module)
      @ Base ./client.jl:419
   [10] #invokelatest#2
      @ ./essentials.jl:729 [inlined]
   [11] invokelatest
      @ ./essentials.jl:727 [inlined]
   [12] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool)
      @ Base ./client.jl:404
   [13] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:318
   [14] _start()
      @ Base ./client.jl:522
ERROR: There was an error during testing
tpapp commented 2 years ago

Thanks!

I am perfectly fine with giving up on any versions below Julia 1.6 if that makes the code simpler, so feel free to just bump compat versions and the lowest CI version and remove the conditionality in tests.

andreasnoack commented 2 years ago

I'll suggest that we more forward with the current version and wait with restricting the supported versions until strictly required. The package still works with older versions and no worse with this PR than before. This is just a performance improvement for more recent versions.

chriselrod commented 2 years ago

I'm okay either way. May be worth testing the fix here on 1.6 as well (I haven't tried on 1.6, so I am not sure it'll work there).

tpapp commented 2 years ago

Thanks, will merge as is. Strictly speaking allocations or inference failures are not considered bugs from my perspective outside the latest released version and LTS.