ARROW-2135: [Python] Fix NaN conversion when casting from Numpy array #1681
Conversation
There was a problem hiding this comment.
For some reason (macro expansion?) these #ifs wouldn't work correctly here, even though NPY_INT64 is defined to NPY_LONG.
There was a problem hiding this comment.
Hmm, actually, that must be because NPY_LONGLONG is not a macro...
pitrou
force-pushed
the
ARROW-2135-nan-conversion-when-casting
branch
2 times, most recently
from
6cbf133 to
d602be7
Compare
pitrou
force-pushed
the
ARROW-2135-nan-conversion-when-casting
branch
3 times, most recently
from
84766bc to
cd37393
Compare
There was a problem hiding this comment.
std::fill(null_bitmap_data_, null_bitmap_data_ + null_bytes, 0) is a bit more idiomatic.
There was a problem hiding this comment.
Hmm, perhaps. This is really a copy/paste of NumPyConverter::InitNullBitmap()...
There was a problem hiding this comment.
Possibly time for a subclass then?
There was a problem hiding this comment.
Is there already a test for things like a = [1.0, 2.0, 3.1, np.nan] where a user passes in an integer type?
There was a problem hiding this comment.
You mean for the truncation behavior? Let me look.
There was a problem hiding this comment.
No, I don't think so. I'm not sure we specify the truncation mode anywhere either?
There was a problem hiding this comment.
It looks like it's a hard cast:
In [7]: pa.array([1, 2, 3.190, np.nan], type=pa.int64())
Out[6]:
<pyarrow.lib.Int64Array object at 0x7f537e42dd68>
[
1,
2,
3,
NA
]
There was a problem hiding this comment.
That's fine. Was just wondering.
cpp/src/arrow/python/type_traits.h
Outdated
There was a problem hiding this comment.
inline is redundant here: http://en.cppreference.com/w/cpp/language/inline.
A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function.
There was a problem hiding this comment.
I see. This is really using the same convention as the rest of the file, though.
There was a problem hiding this comment.
Hm, so that's also called isnull. Shouldn't that mean v == Py_None?
There was a problem hiding this comment.
Probably needs a test as well since it isn't failing.
There was a problem hiding this comment.
Nice catch :-) I'm not sure how to test it. Defining isnull is necessary for compiling, but that path isn't taken at runtime as object arrays are handled separately.
There was a problem hiding this comment.
At some point we may want to have an STL-compatible view class that makes interacting with iterators constructs in the STL much easier. We have a lot of code that is manually handling iteration using a size/count and a buffer.
There was a problem hiding this comment.
Which iterators are you thinking about? Do you mean the ndarray 1d iterator?
There was a problem hiding this comment.
That's one, though I added begin()/end() for that in #1651.
pitrou
force-pushed
the
ARROW-2135-nan-conversion-when-casting
branch
3 times, most recently
from
73916de to
bb56637
Compare
There was a problem hiding this comment.
By the way, I don't know what that is, but this is required to have the tests pass. Why do we always treat NaT as null but not floating-point NaN? @wesm
There was a problem hiding this comment.
AFAIU There's no other way to interpret NaT other than NULL (unless there's a standard that defines it in a different way than "missing"). nan is part of the IEEE floating point specification (as I'm sure you know) and it has a different meaning than null.
|
I addressed some review comments now. |
pitrou
force-pushed
the
ARROW-2135-nan-conversion-when-casting
branch
from
bb56637 to
375418f
Compare
|
AppVeyor build at https://ci.appveyor.com/project/pitrou/arrow/build/1.0.157 |
pitrou
force-pushed
the
ARROW-2135-nan-conversion-when-casting
branch
from
375418f to
0af573b
Compare
pitrou
force-pushed
the
ARROW-2135-nan-conversion-when-casting
branch
from
0af573b to
939428d
Compare
|
Rebased. |
wesm
left a comment
wesm
left a comment
There was a problem hiding this comment.
+1, thanks for cleaning up the int/uint size issues here, much cleaner now
|
see ARROW-2298 for adding an option about NaN conversions |
3 participants
No description provided.