ENH: ignoring comment lines and empty lines in CSV files

[mdmueller]

closes #4466

[jreback]

@AmrAS1 thanks for taking this on! lmk if you need any help

we could put this in 0.14.1 if you can do this in next 2 weeks

[mdmueller]

@jreback - No problem! I should be done with this pretty soon. Right now I'm just getting test failures with the Python parsing engine, so I'll add this functionality to PythonParser.

[jreback]

• does this cover partial whitespace in the beginning? (e.g. say "\t\t \n") ?
  (obviously sep is not whitespace).
• I see that you cover comment char at non-zero location
  check for >1 char of comment char as well (I think its really problematic to allow it to be a reg-ex).
• pls run a perf check (and post if anything looks odd)
• release note in v0.14.1.txt (note that I changed the above from the solving PR to the actual issue, use these 2 issues in the release notes)

thanks!

[jreback]

will this close #4623 ?

[mdmueller]

This will close #4623 according to current output:

In [3]: s1 = '# notes\na,b,c\n# more notes\n1,2,3'

In [4]: s2 = 'a,b,c\n# more notes\n1,2,3'

In [5]: pd.read_csv(StringIO(s1), comment='#')
Out[5]: 
   a  b  c
0  1  2  3

In [6]: pd.read_csv(StringIO(s2), comment='#')
Out[6]: 
   a  b  c
0  1  2  3

In [7]: pd.read_csv(StringIO(s1), comment='#', header=None)
Out[7]: 
   0  1  2
0  a  b  c
1  1  2  3

I'll check out the "\t\t \n" case -- I think this would have to be addressed after tokenization in the Cython code, unless there's a better way. I'm a little confused about what you mean by checking for >1 char of the comment char though, doesn't the C parser disallow regex comments anyway?

[jreback]

what I mean is you should disallow comment='comment' , e.g. a comment string with len > 1 (I think this would be very tricky and not necessary), just raise a ValueError.

for the \t\t just trying to think of cases (I agree its prob handled but not sure if their are tests for something like this, e.g. whitespace preceding a line terminator (and nothing else)

[mdmueller]

Unless I'm missing something, it looks like that's the current functionality already:

In [9]: pd.read_csv(StringIO(s1), comment='##', header=None)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-9-455c49b9113b> in <module>()
----> 1 pd.read_csv(StringIO(s1), comment='##', header=None)

/home/michael/git/pandas/pandas/io/parsers.pyc in parser_f(filepath_or_buffer, sep, dialect, compression, doublequote, escapechar, quotechar, quoting, skipinitialspace, lineterminator, header, index_col, names, prefix, skiprows, skipfooter, skip_footer, na_values, na_fvalues, true_values, false_values, delimiter, converters, dtype, usecols, engine, delim_whitespace, as_recarray, na_filter, compact_ints, use_unsigned, low_memory, buffer_lines, warn_bad_lines, error_bad_lines, keep_default_na, thousands, comment, decimal, parse_dates, keep_date_col, dayfirst, date_parser, memory_map, nrows, iterator, chunksize, verbose, encoding, squeeze, mangle_dupe_cols, tupleize_cols, infer_datetime_format)
    441                     infer_datetime_format=infer_datetime_format)
    442 
--> 443         return _read(filepath_or_buffer, kwds)
    444 
    445     parser_f.__name__ = name

/home/michael/git/pandas/pandas/io/parsers.pyc in _read(filepath_or_buffer, kwds)
    226 
    227     # Create the parser.
--> 228     parser = TextFileReader(filepath_or_buffer, **kwds)
    229 
    230     if nrows is not None:

/home/michael/git/pandas/pandas/io/parsers.pyc in __init__(self, f, engine, **kwds)
    531             self.options['has_index_names'] = kwds['has_index_names']
    532 
--> 533         self._make_engine(self.engine)
    534 
    535     def _get_options_with_defaults(self, engine):

/home/michael/git/pandas/pandas/io/parsers.pyc in _make_engine(self, engine)
    668     def _make_engine(self, engine='c'):
    669         if engine == 'c':
--> 670             self._engine = CParserWrapper(self.f, **self.options)
    671         else:
    672             if engine == 'python':

/home/michael/git/pandas/pandas/io/parsers.pyc in __init__(self, src, **kwds)
   1030         kwds['allow_leading_cols'] = self.index_col is not False
   1031 
-> 1032         self._reader = _parser.TextReader(src, **kwds)
   1033 
   1034         # XXX

/home/michael/git/pandas/pandas/parser.so in pandas.parser.TextReader.__cinit__ (pandas/parser.c:3607)()

ValueError: Only length-1 comment characters supported

The tricky thing with the whitespace is for cases like '\t\t ,\t' or '\t a' where the delimiter or non-whitespace show up eventually. I guess I could backtrack in the tokenization algorithm like how \r line terminators are handled, would that be preferable to post-processing?

[jreback]

@AmrAS1 if their is a test for multi-char comments then good

I think handling the back-tracking similar to \r is good (don't want to slow up the general case).

[mdmueller]

@jreback - I'm almost done, but I found a test failure involving a file which ends with a bunch of tab-filled lines using read_table (which defaults to sep='\t'). Would the correct behavior be not to ignore these lines, i.e. to count any line including the delimiter?

[jreback]

I think if u have the delimiter then the line counts

[mdmueller]

@jreback - I think I'm done here, but let me know if there are any problems.

[jreback]

say it more like: read_csv/read_table file parsers now ignore comments provided with the comment=.... (say something about single character), and ignore starting at the comment

[jreback]

pls run a perf check (test_perf.sh -b master -t HEAD) to see if anything > 1.2 shows up (if it does pls post it and investigate)

pls squash this down to a small number of commits

otherwise looks good!

[mdmueller]

There were two with a ratio >1.2 but both are unrelated to parsing. Here is the bottom of the results:

dtype_infer_int64                            |   1.0494 |   0.9537 |   1.1003 |
stats_rank_average                           |  22.0106 |  19.8573 |   1.1084 |
dtype_infer_int32                            |   0.3977 |   0.3580 |   1.1108 |
dti_reset_index                              |   0.2803 |   0.2510 |   1.1168 |
dtype_infer_float64                          |   0.9977 |   0.8846 |   1.1278 |
indexing_dataframe_boolean_rows              |   0.2737 |   0.2414 |   1.1340 |
frame_ctor_dtindex_YearBeginx1               |   0.7073 |   0.6123 |   1.1551 |
groupby_multi_different_numpy_functions      |   9.1350 |   7.7774 |   1.1746 |
merge_2intkey_nosort                         |  12.1953 |   9.9773 |   1.2223 |
groupby_simple_compress_timing               |  22.3273 |  15.4157 |   1.4484 |

I assume the slower performance is due to previous commits. I'll squash the commits on this PR now.

[jreback]

perf looks fine

[jreback]

you have some failing tests

[mdmueller]

Looks like it was the same error, I fixed it in the last commit so now I'm waiting on Travis to make sure.

[mdmueller]

Hm, it's fixed now except for an error in test_round_trip_exception_ under test_json, which calls read_csv('https://raw.github.com/hayd/lahman2012/master/csvs/Teams.csv'). I tried it locally and it works fine, not sure what that's about. I'll check it out.

[jreback]

@AmrAS1 hows' this coming along?

[mdmueller]

@jreback - I tried running Travis on my repo with these changes and it worked except for a timeout. Not sure what's up, is there any way we can restart Travis on this PR?

[jreback]

restarted

do the doc-string / docs need updating
maybe add a small section on comments? (if it's not obvious from doc string)

[mdmueller]

Updated -- Travis seems to be stuck on a timeout.

[jreback]

push this up again

git commit -C HEAD --amend
then force push

it will force a new travis build

[mdmueller]

Travis is failing for some odd reason that seems unrelated to recent changes. @jreback do you know what this could be?

[jreback]

those are seg faults
check and see if it works locally

[mdmueller]

It works locally for me

[jreback]

so a way to debug is I have nosetests output each test (to see where it is failing)

add the -v argument in ci/script.py (only temporary of course)

[mdmueller]

Ah, must've forgotten to recompile extensions. The problem was that I forgot to change the parameter name in parser.pyx, I'll amend this.

[jreback]

This is an API change (that blank lines will be skipped). Maybe make a warning here that this is new in 0.15?

[mdmueller]

Ok added API change notice

[jorisvandenbossche]

Maybe mention explicitely that it is read_csv (and read_table, I suppose)? That is more clear for users I think.

[mdmueller]

Amended to add this in

[jreback]

pls rebase and squash

[mdmueller]

@jreback - rebased and squashed

[jreback]

put the default here (True)

[jreback]

@AmrAS1 couple of minor documenting changes. ping when when green.

[mdmueller]

@jreback - Green

[jreback]

side issue I happen to have an old version of cython installed on 1 of my machines (0.17.1). didn't compile the parser.pyx correctly, but the newer ones do. odd - so going to change the min version (to 0.19.1) which is still pretty old

[mdmueller]

Interesting, do you know what the compile error was?

[jreback]

merged via 31c2558

[jreback]

I broke on the float_precision pointer-to-func (was a formatting error). 0.19.1 works fine though

[jorisvandenbossche]

@amanshei Did you got it sorted out?

[amanshei]

I did, thank you!