BUG: Patch handling of keep_default_na=False #19260
Conversation
| the default NaN values are used for parsing. | ||
| * If `keep_default_na` is False, and `na_values` are not specified, only | ||
| the NaN values specified `na_values` are used for parsing. | ||
| * If `keep_default_na` is False, and `na_values` are not specified, no |
There was a problem hiding this comment.
is this the same as the 3rd bullet?
There was a problem hiding this comment.
No, that first line is a just copy-paste fail.
doc/source/io.rst Outdated
| If na_values are specified and keep_default_na is ``False`` the default NaN | ||
| values are overridden, otherwise they're appended to. | ||
| Whether or not to include the default NaN values when parsing the data. | ||
| Provided that `na_filter` is True, depending on whether `na_values` is |
There was a problem hiding this comment.
the 2nd sentence is slightly confusing here
pandas/tests/io/parser/na_values.py Outdated
| 'seven']}) | ||
| tm.assert_frame_equal(xp.reindex(columns=df.columns), df) | ||
| | ||
| # see gh-19227: keep_default_na=False should be enforced |
There was a problem hiding this comment.
can you split this test up, getting a bit long
gfyoung force-pushed the na-values-csv branch from 8ef3ae5 to 9cc889c Compare | If na_values are specified and keep_default_na is ``False`` the default NaN | ||
| values are overridden, otherwise they're appended to. | ||
| Whether or not to include the default NaN values when parsing the data. | ||
| Depending on whether `na_values` is passed in, the behavior is as follows: |
| elif isinstance(na_values, dict): | ||
| na_values = na_values.copy() # Prevent aliasing. | ||
| if keep_default_na: | ||
| for k, v in compat.iteritems(na_values): |
There was a problem hiding this comment.
might be nice to have a comment or 2 to explain the logic here for future readers
gfyoung force-pushed the na-values-csv branch from 9cc889c to dd5880f Compare 
Codecov Report
@@ Coverage Diff @@ ## master #19260 +/- ## ========================================== + Coverage 91.56% 91.56% +<.01% ========================================== Files 148 148 Lines 48874 48878 +4 ========================================== + Hits 44751 44755 +4 Misses 4123 4123
Continue to review full report at Codecov.
|
| (k, _floatify_na_values(v)) for k, v in na_values.items() # noqa | ||
| ) | ||
| | ||
| na_values[k] = v |
There was a problem hiding this comment.
this is an anti-pattern to modify the dict that you are iterating. can you create you create a new one here?
There was a problem hiding this comment.
@jreback : I realize that, but I think that's why up above, someone wrote na_values = na_values.copy(). The reference to the original input is destroyed and created a "new" dictionary. Do you just want me to change the assigned variable name?
There was a problem hiding this comment.
yeah you should create a new empty dict and then assign to it (it doesn't have to be a copy). iterating and modifying is a no-no.
There was a problem hiding this comment.
I've decided to create a copy because at the bottom, we return na_values and na_fvalues regardless of the logic branch. However, I'll iterate over the old_na_values instead.
gfyoung force-pushed the na-values-csv branch from dd5880f to ecbe462 Compare 
Patches very buggy behavior of keep_default_na=False whenever na_values is a dict * Respect keep_default_na for column that doesn't exist in na_values dictionary * Don't crash / break when na_value is a scalar in the na_values dictionary. In addition, clarifies documentation on behavior of keep_default_na with respect to na_filter and na_values. Closes pandas-devgh-19227.
gfyoung force-pushed the na-values-csv branch from ecbe462 to 63faf8d Compare 
| @jreback : The OSX builds on Travis seemed to have stalled completely (all of the other tests have been done for a LONG time now). |
| thanks @gfyoung |
2 participants
Patches very buggy behavior of
keep_default_na=Falsewheneverna_valuesis a dictkeep_default_nafor column that doesn't exist inna_valuesdictionaryna_valueis a scalar in thena_valuesdictionary.In addition, clarifies documentation on the handling of the keep
keep_default_naparameter with respect tona_filterandna_values.Closes #19227.
cc @neilser