BUG: Patch read_csv NA values behaviour #14751
Conversation
Current coverage is 85.27% (diff: 100%)
|
gfyoung
force-pushed
the
csv-na-values-patching
branch
2 times, most recently
from
8ebba67 to
b9b0367
Compare
|
Appveyor keeps failing because the build is taking too long, but I have no clue why that is the case (e.g. this build seemed to do just fine). I took a look at Travis to see if the builds much longer, but that is not the case. |
gfyoung
force-pushed
the
csv-na-values-patching
branch
2 times, most recently
from
a2a5b63 to
f086bac
Compare
gfyoung
force-pushed
the
csv-na-values-patching
branch
2 times, most recently
from
5bd7c10 to
0bd7531
Compare
| clean_na_values = {} | ||
| clean_na_fvalues = {} | ||
|
|
||
| if isinstance(self.na_values, dict): |
There was a problem hiding this comment.
I think it might be better to have a single function in parsers.pyx which does this (e.g. returns cleaned na_value, na_fvalue). rather than have duplicate code in python and c parser.
There was a problem hiding this comment.
Hmmm...perhaps, but the logic is very coupled with the class attributes of the C and Python parsers. In addition, the way the logic is applied (in bulk for Python, per iteration in C), merging the two would not be straightforward.
There was a problem hiding this comment.
(in bulk for Python, per iteration in C),
what does this mean?
this should be done once
There was a problem hiding this comment.
Look at the _get_na_list method here for the C engine. It gets called on every iteration to extract the relevant na_values for the particular column in question.
There was a problem hiding this comment.
that's only called on column conversion (once).
There was a problem hiding this comment.
There really is no difference. All we do is get the na_values for the given column, "clean it" and then use them to parse the rest of the column. In Python, we get the na_values for all relevant columns, clean them, and then use stored dictionary later on.
There was a problem hiding this comment.
just trying to reduce the amount of code
having a common method for all parsers is better than having specific things for each
There was a problem hiding this comment.
No, I perfectly understand. I just haven't found a clean way to do it yet, and it seems like it would be best to do as a follow-up if that works.
There was a problem hiding this comment.
oh ok
let's do that then
lgtm
There was a problem hiding this comment.
Great. I'll ping again when everything goes green.
gfyoung
force-pushed
the
csv-na-values-patching
branch
from
0bd7531 to
cac422c
Compare
gfyoung
changed the title
|
@jreback : Everything is passing. Ready to merge if there are no other concerns. |
|
thanks! |
Patches the following behaviour when `na_values` is passed in as a dictionary: 1. Prevent aliasing in case `na_values` was defined in a broader scope. 2. Respect column indices as keys when doing NA conversions. Closes pandas-dev#14203. Author: gfyoung <[email protected]> Closes pandas-dev#14751 from gfyoung/csv-na-values-patching and squashes the following commits: cac422c [gfyoung] BUG: Respect column indices for dict-like na_values 1439c27 [gfyoung] BUG: Prevent aliasing of dict na_values
Patches the following behaviour when `na_values` is passed in as a dictionary: 1. Prevent aliasing in case `na_values` was defined in a broader scope. 2. Respect column indices as keys when doing NA conversions. Closes pandas-dev#14203. Author: gfyoung <[email protected]> Closes pandas-dev#14751 from gfyoung/csv-na-values-patching and squashes the following commits: cac422c [gfyoung] BUG: Respect column indices for dict-like na_values 1439c27 [gfyoung] BUG: Prevent aliasing of dict na_values (cherry picked from commit dd8cba2)
Patches the following behaviour when
na_valuesis passed in as a dictionary:na_valueswas defined in a broader scope.Closes #14203.