NullableArrays.jl icon indicating copy to clipboard operation
NullableArrays.jl copied to clipboard

`UndefVarError` when returning `Nullable` from `map` with `lift=true`

Open ExpandingMan opened this issue 8 years ago • 3 comments

X = NullableArray(1:100)
map(x -> (x == 2 ? Nullable() : x), X, lift=true)

gives the following error

ERROR: UndefVarError: map_to! not defined
 in _liftedmap_to!(::##3#4, ::NullableArrays.NullableArray{Int64,1}, ::NullableArrays.NullableArray{Int64,1}, ::Int64, ::Int64) at /home/savastio/.julia/v0.5/NullableArrays/src/map.jl:198
 in _liftedmap(::##3#4, ::NullableArrays.NullableArray{Int64,1}) at /home/savastio/.julia/v0.5/NullableArrays/src/map.jl:140
 in #map#15 at /home/savastio/.julia/v0.5/NullableArrays/src/map.jl:113 [inlined]
 in (::Base.#kw##map)(::Array{Any,1}, ::Base.#map, ::Function, ::NullableArrays.NullableArray{Int64,1}) at ./<missing>:0

I suppose this may be something that simply hasn't been implemented yet, but if so it probably should have returned an error that said so.

Changing the returned Nullable() to be type specific does not help.

ExpandingMan avatar Oct 21 '16 13:10 ExpandingMan

I think the idea behind lift=true is that you're supposed to get non-null values and return non-null values. There's no reason not to support this use case, though.

nalimilan avatar Oct 21 '16 13:10 nalimilan

To give you an example of where it comes up: I decided that I wanted to convert all NaNs to Nullable{Float64}() for the sake of consistency. To do this without lift one would need

map(X) do x
    if !isnull(x) && isnan(get(x))
        return Nullable()
    end
    return x
end

Actually I think this is pretty typical of the design challenge facing this package: either most functions have to be overloaded to deal with Nullable (though there isn't always a perfectly consistent way of doing so) or there need to be iterators that make it easier for non-specialized functions to get applied. It's probably not an unusual thing for non-null values to get converted to null, and in those cases you still don't really want to make special accommodations for existing nulls.

ExpandingMan avatar Oct 21 '16 14:10 ExpandingMan

lift=true is just supposed to enable standard lifting semantics for whatever f is passed to map --- precisely the behavior that @ExpandingMan was expecting. I think that what is happening here is that, since there are no nulls in X, map is trying to fall back to an internal map_to! implementation from Base that, evidently, no longer exists.

EDIT: This probably hasn't existed in Base for a while now, actually. We probably are missing this case in our tests.

Also, needless to say, this is yet another reason to just get rid of our map and broadcast implementations.

davidagold avatar Oct 21 '16 15:10 davidagold