Make inverse(::VectorTransfrom, x) return a vector of floats

[devmotion]

Fixes #132.

I was debating with myself whether this should be addressed in inverse or inverse_eltype. I came to the conclusion that inverse is the right place because modifications in inverse_eltype could lead to undesired results (too early change to floats or Float64) when composing vector transforms (and the current inverse_eltype definitions are not actually incorrect).

[tpapp]

I think that this catches too much. I would simply condition on dimension(t) == 0, I don't think that Union{} can happen outside that. Something like

T = inverse_eltype(t, y)
d = dimension(t)    
if T === Union{} || d == 0
    T = Float64
end
inverse!(Vector{T)}(undef, d), t, y)

[devmotion]

In your suggestion you seem to condition on Union{} as well?

I was actually considering checking only dimsnion(t) == 0 as for my use case it shouldn't matter. I went with checking only whether inverse_eltype(t, y) === Union{} since I was worried that branching on the value of dimension(t) could introduce a type instability.

[tpapp]

Yes, that makes sense.

But now that I think about it, it is kind of funny to make eltype(inverse(t, y)) != inverse_eltype(t, y). I think it would be best to fix it in inverse_eltype, the following way:

1. Introduce a function, eg narrow_inverse_eltype, basically rename inverse_eltype to that, in the state before this PR. Document that types should extent it.

2. In inverse_eltype, just check for Union{} and replace with Float64.

Now that I look at the code, the eltype determination is a bit flaky in a lot of places:

julia> t = as(Array, 3)
[1:3] 3×
  asℝ

julia> inverse(t, Any[1, 2.1, 3])
ERROR: InexactError: Int64(2.1)

We should probably rethink it as a whole for all kinds of corner cases. Suggestions welcome.

[devmotion]

Initially I wanted to change inverse_eltype. But making it concrete seemed wrong - similar to the scalar transforms (such as inverse_eltype(::Constant) = Union{}), per se an inverse_eltype seems completely correct: It avoids prematurely introducing e.g. Float64 in a "non-leaf" transform (ie before applying all compositions or combinations). IMO the element type is only relevant when the output array is actually instantiated, and changes (such as changing to floating point number types or turning Union{} to Float64) should only be performed at this final stage - as done in this PR.

Now that I look at the code, the eltype determination is a bit flaky in a lot of places:

I think this example is quite special as it is caused by the same bug as #73: The element type of the output (inverse_eltype) is in this case computed based on the first element of input x. Whereas actually for non-concrete element types, I think inverse_eltype take into account the full input x, e.g. by promoting the inverse_eltypes for each element (can't be inferred in this case) or returning an abstract type such as Any.

julia> TransformVariables.inverse_eltype(t, Any[1, 2.1, 3])
Int64

julia> TransformVariables.inverse_eltype(t, Any[2.1, 1, 3])
Float64

By construction of this failing example, it is actually fixed by this PR since Int would be changed to Float64 when constructing the Vector. With this PR,

julia> inverse(t, Any[1, 2.1, 3])
3-element Vector{Float64}:
 1.0
 2.1
 3.0

[tpapp]

LGTM, thanks! I will clarify the API in another PR.

[devmotion]

Could you tag a new release? I updated the package version in this PR but you might want to also include the latest commit on the master branches with the additional explanations regarding inverse_eltype.

[tpapp]

I am away from my compurer for today, please tag one (I added you to the repo).

[devmotion]

OK, will do!