Repository navigation
Surprising behaviors in array initialization #17
Description
The documentation says:
For isbits types, a random/reasonable sentinel will be attempted if none provided.
...
ForIntegertypes, all bit patterns are valid, so a random value is chosen;
But the constructor uses a constant default value. For example the default sentinel for Int is -1, so SentinelArray([-1, -1, -1]) creates an array with three missing values. From the documentation I expected an array of three -1, and a random sentinel different from -1.
The behavior with undef is also surprising: The documentation refers to the "standard undef pattern". As I understand, undef is meant to create uninitialized arrays, for cases where initialization is useless and undesired for performance reasons. But SentinelVector{Int}(undef, 3) actually initializes the array with 3 missing values. I would suggest removing the undef argument from the constructors (so a vector full of 3 missing values can be created with SentinelVector{Int}(3)). A later version could re-introduce the undef pattern with the correct semantics...
Activity
quinnj commentedon Oct 8, 2020
I don't think the behavior with
undefis surprising; it follows the same pattern as if you didVector{Union{Float64, Missing}}(undef, 10). Though we are inconsistent with ref types whereVector{Union{String, Missing}}(undef, 10)will give you#undefandSentinelVector{String}(undef, 10)will give youmissing, but I think that's permissible sinceSentinelVectoris really about data and we're essentially saying the default for "missing" values ismissing, not#undef.There is an issue with passing in your own arrays to
SentinelArraywhen they contain the sentinel, because then they're treated asmissing(or whatever sentinel you want). This is tricky because I think there are use-cases both ways: you have data, that has a sentinel value already (think-999or something) and so you callSentinelArray(array, -999)and boom; it's fast, easy, and now you're treating-999asmissing. But if you have a data array like[-1, 0, 1, 2, 3]and callSentinelArray(array)then the first element is now treated asmissing, probably unintentionally. We should probably have a keyword argument that allows "passing through" the data, which means we'll check the incoming data for any sentinel values and cycle the default sentinel if present. Anyone have any good ideas for what to call that keyword argument? cc: @bkamins @nalimilanbkamins commentedon Oct 9, 2020
maybe
raw::Bool? iftrueassume no processing, iffalsethen we process the array.sijow commentedon Oct 9, 2020
Interesting! This behavior is undocumented though. Here's what I found by looking at discussions in Julia issues:
There was an attempt to document this behavior: Docs of construction undef arrays with missing JuliaLang/julia#31091. But the conclusion was rather to document it as undefined or type-dependent. Indeed it's not reliable for singleton types:
The undef initializer is needed for performance reasons (see comments starting at RFC: Get rid of #undef and replace it with null in Array{Union{Null, T}} JuliaLang/julia#23721 (comment)). That's what I would expect given the documented semantics (and the name).
There was a need for a nice way to initialize arrays with missing values: Improve Array{Union{T, Missing}} constructors JuliaLang/julia#24939 . This was solved by defining a new
Array{T,N}(missing, dims)constructor: Add Array{T,N}(nothing/missing, dims) convenience constructors JuliaLang/julia#25054.So what about replacing
undefwithmissing?nalimilan commentedon Oct 9, 2020
I think the behavior with
undefis fine, as you noted it's the same as in Base (and e.g.CategoricalArraydoes the same thing). But it would make sense to supportSentinelArray{T,N}(missing, dims)too to parallel theArrayconstructor.I agree that would be safer. You could also consider that if the sentinel isn't passed explicitly, then the caller doesn't know it, so you should check whether it's in the data, and if so choose a different random sentinel. OTC when the sentinel is explicitly passed, then the caller knows that such values will be treated as missing if they appear in the data.
Why is
-1used as the sentinel by default? It sounds much more likely to clash with existing values than any other (large) random value.quinnj commentedon Oct 9, 2020
So we can initialize the SentinelArray faster via
ccall(:memset, Ptr{Cvoid}, (Ptr{Cvoid}, Cint, Csize_t), pointer(A), 0xff, sizeof(A)); otherwise, we need to iterate each element and set the sentinel value. (i.e. only possible if sentinel has a uniform bit pattern, so0or-1)Yeah, I like the idea of doing it differently if the user is passing the sentinel or not. Let me look into what that change looks like.
sijow commentedon Oct 10, 2020
It's not terrible, but it seems wasteful to conflate the
missingandundefconstructors since they have different semantics. One is about initializing an array with missing values, the other is about skipping initialization for performance reasons. I'd rather have anundefconstructor that gives the performance boost.I also see little value in having two ways to do the same thing, one being the "official" way of the core language, and another way that encourages to use an anti-pattern: abusing the
undefconstructor to have defined (missing) values. I say anti-pattern because in Base it relies on an undocumented implementation detail, and it works almost always but not always (perfect for nasty bugs).It seems clear that the current design is due to historical reasons (there was no
missingArray constructor in Base, and people hadn't realized that the undef-for-missing behavior in Base was not quite reliable). Why not make the API cleaner and more expressive before the 1.0 release? If CategoricalArray does the same, then let's fix it too!nalimilan commentedon Oct 10, 2020
I don't think that's the case at all. AFAICT the reason why
Vector{Union{Missing, Int}}(undef, dims)returns a vector full ofmissingis that the type tag part of the underlying storage has to be initialized to a valid type (eitherIntorMissing). The default happens to beMissingin most cases due to how types are sorted, and since for that type the value doesn't matter since it's a singleton, you getmissing. But that doesn't affect performance at all so it's perfectly fine.The same applies to CategoricalArrays, where we also need to fill reference codes with a valid value (otherwise we would have to check that they are in the expected range all the time which would require two comparisons instead of one).
0is used, which indicates#undefforCategoricalVector{Int}but indicatesmissingforCategoricalVector{Union{Missing, Int}}.bkamins commentedon Oct 10, 2020
I confirm that this is also my understanding.
sijow commentedon Oct 10, 2020
I agree 100% (by "current design" I meant that of SentinelArrays, not Base). I'm not arguing against the behavior in Base. And I see now that the behavior of SentinelArrays for
undefis not that confusing, so this issue is not the right place to discuss the point I was trying to make in my previous 2 comments. Sorry for the confusion, I'll try to make a clearer case for my point in another issue.bkamins commentedon Oct 10, 2020
Your comments are valuable - we just try very hard to maintain consistency with Base where possible so it is better to discuss things in detail.
sijow commentedon Oct 10, 2020
Thanks @bkamins, that makes sense. And I appreciate the time you all take to answer my proposals. I have made a hopefully clearer case in a new issue: #40.