BUG: Avoid flaky usecols set in C engine #14984
Conversation
Current coverage is 84.78% (diff: 100%)@@ master #14984 diff @@
==========================================
Files 145 145
Lines 51090 51094 +4
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 43315 43319 +4
Misses 7775 7775
Partials 0 0
|
| usecols_dtype = lib.infer_dtype(usecols) | ||
| if usecols_dtype not in ('empty', 'integer', | ||
| 'string', 'unicode'): | ||
| raise ValueError(msg) | ||
|
|
||
| return set(usecols) | ||
| return usecols | ||
| return set(usecols), usecols_dtype |
There was a problem hiding this comment.
why does this still have a set conversion here? is this for a different case? if so, then should happen in your validation below (to make it more consistent), IOW, this always returns tuple of (list of usecols, dtype of usecols). Please make the doc-string indicate that as well (t he doc-string for this function)
There was a problem hiding this comment.
I'm not sure I fully follow your comment here. The reason I return set is for the Python engine. I can clarify the doc-string to return set (or None) along the dtype of usecols.
There was a problem hiding this comment.
I think it would be more clear to have _validate_use_cols always return (list, dtype) (for all engines). It makes it very clear what it does. Then have it convert to a set elsewhere (as you do in _set_noconvert_columns. otherwise you have 2 places which do this conversion (from list -> set), depending on the engine. again confusing.
There was a problem hiding this comment.
But it's always set for both engines. This function returns either (independent of engine):
setNonecallable
There was a problem hiding this comment.
@jreback : I updated the documentation to illustrate the return type is independent of engine.
There was a problem hiding this comment.
still, why are we returning a set here? why not a list? (for this same case)? then you don't have to rely on the ordering and convert to a list if its integer type?
could just do this: http://stackoverflow.com/questions/480214/how-do-you-remove-duplicates-from-a-list-in-whilst-preserving-order
then make it into a set at the last moment.
I am nitpicking on this because converting this to a set is happening too early, and it is just confusing.
There was a problem hiding this comment.
Early conversion to set has been used for quite some time, especially with the C engine. Also, a list is not necessarily going to help with ordering I might add:
>>> from pandas import read_csv
>>> from pandas.compat import StringIO
>>>
>>> data = 'a,b,c\n1,2,3'
>>> read_csv(StringIO(data), usecols=[2, 1])
b c
0 2 3When I say "order," it is not about maintaining the order in which usecols is passed in but about ordering usecols correctly so that the names or indices are in the same order as they are listed in the data columns. That is why converting at the last moment does not present any benefit AFAICT.
gfyoung
force-pushed
the
usecols-parse-dates-list
branch
from
30bae02 to
6ac5814
Compare
| if self.usecols_dtype == 'integer': | ||
| # A set of integers will be converted to a list in | ||
| # the correct order every single time. | ||
| usecols = list(self.usecols) |
There was a problem hiding this comment.
so this is very confusing here. You are now converting to a list. So usecols ends up as a list, None or a callable? when / how does it end up as a set?
There was a problem hiding this comment.
Can you add a short docstring for this _set_noconvert_columns function?
There was a problem hiding this comment.
@jreback : During __init__, both functions call _validate_usecols_arg when setting self.usecols, which returns either a callable, set, or None.
@jorisvandenbossche : Added docstring.
gfyoung
force-pushed
the
usecols-parse-dates-list
branch
from
6ac5814 to
82cf55b
Compare
|
@jreback , @jorisvandenbossche : Addressed comments, and everything is green. |
|
ok thanks @gfyoung |
Explanation of the bug can be found here.
Closes #14792.