Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

UndefVarError when returning Nullable from map with lift=true #163

Open
ExpandingMan opened this issue Oct 21, 2016 · 3 comments
Open

UndefVarError when returning Nullable from map with lift=true #163

ExpandingMan opened this issue Oct 21, 2016 · 3 comments

Comments

@ExpandingMan
Copy link

ExpandingMan commented Oct 21, 2016

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.

@nalimilan
Copy link
Member

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.

@ExpandingMan
Copy link
Author

ExpandingMan commented Oct 21, 2016

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.

@davidagold
Copy link
Contributor

davidagold commented Oct 21, 2016

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants