BUG: Parse uint64 in read_csv #15020
Conversation
gfyoung
force-pushed
the
read-csv-uint64
branch
from
e4c204f to
e4e3c10
Compare
Current coverage is 84.77% (diff: 100%)@@ master #15020 diff @@
==========================================
Files 145 145
Lines 51131 51131
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 43345 43345
Misses 7786 7786
Partials 0 0
|
gfyoung
force-pushed
the
read-csv-uint64
branch
from
e4e3c10 to
0e11b64
Compare
jreback
added
Bug
Dtype Conversions
|
pls run a perf check (maybe need some additonal asvs) |
Adds behavior to allow for parsing of uint64 data in read_csv. Also ensures that they are properly handled along with NaN and negative values. Closes pandas-devgh-14983.
gfyoung
force-pushed
the
read-csv-uint64
branch
from
0e11b64 to
d018e38
Compare
|
@jreback : Added one benchmark, and performance does not seem impacted significantly (if at all). |
| @@ -288,6 +288,7 @@ Bug Fixes | |||
| - Bug in ``Index`` power operations with reversed operands (:issue:`14973`) | |||
| - Bug in ``TimedeltaIndex`` addition where overflow was being allowed without error (:issue:`14816`) | |||
| - Bug in ``DataFrame`` construction in which unsigned 64-bit integer elements were being converted to objects (:issue:`14881`) | |||
| - Bug in ``pd.read_csv()`` in which unsigned 64-bit integer elements were being improperly converted to the wrong data types (:issue:`14983`) | |||
There was a problem hiding this comment.
We could also call this an enhancement that read_csv now supports reading uint64 values?
There was a problem hiding this comment.
IMO this is a bug because read_csv should be able to handle all data types equally. In addition, both the issue and this PR have already been tagged as a bug.
There was a problem hiding this comment.
It doesn't really matter whether it is a bug fix or not (or how we tag the PR), what I just wanted to say is that we can highlight it more by putting it in the enhancement section, if we think it is worth it.
There was a problem hiding this comment.
Ah, fair enough. I just realized though: #14937 (see my changes to thewhatsnew) might actually resolve this discussion?
There was a problem hiding this comment.
yep, indeed, we can group all uint enhancement/bug fixes together
There was a problem hiding this comment.
Okay, so we can leave that for #14937 then. 😄
|
@jreback : Any other comments about this PR? Otherwise, seems ready to merge. |
|
@gfyoung haven't looked yet. soon. |
| @@ -1750,6 +1772,78 @@ cdef inline int _try_double_nogil(parser_t *parser, int col, | |||
|
|
|||
| return 0 | |||
|
|
|||
| cdef _try_uint64(parser_t *parser, int col, int line_start, int line_end, | |||
| bint na_filter, kh_str_t *na_hashset): | |||
There was a problem hiding this comment.
prob for another PR, but we should generate these try_* routines with tempita (if possible)
There was a problem hiding this comment.
It's possible I imagine but not as nice since the implementations are definitely customized to the dtype (e.g. note how I did not pass in any NA value for uint64, but the int64 implementation does)
| @@ -1876,3 +1886,88 @@ int64_t str_to_int64(const char *p_item, int64_t int_min, int64_t int_max, | |||
| *error = 0; | |||
| return number; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
maybe generate some code here too (for another PR)
There was a problem hiding this comment.
Again, possible but not as nice because the implementations are definitely customized to the dtype (e.g. compare the difference in handling for "negative numbers" for int64 and uint64).
|
thanks @gfyoung my comments are for future code generation. |
|
@jreback : Yep, got it. Thanks! |
Adds behavior to allow for parsing of
uint64data inread_csv. Also ensuresthat they are properly handled along with
NaNand negative values.Closes #14983.