ENH: ignoring comment lines and empty lines in CSV files
State: closed

closes #4466 


Labels:
name: Enhancement
name: IO CSV
description: read_csv, to_csv

Comments:
@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

@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`.

- 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!

will this close #4623 ?

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?

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)

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?

@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).

@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?

I think if u have the delimiter then the line counts

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

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!

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.

perf looks fine

you have some failing tests

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

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.

@amras1 hows' this coming along?

@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?

restarted 

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

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

push this up again

`git commit -C HEAD --amend`
then force push

it will force a new travis build

Travis is still halting on the same build (python 2.7, designated as "slow and not network and not disabled"). What should I do?

hmm.something IS hanging. 

duplicate the environment (its has LOCALE stuff going on, which may be the reason). in a virtual env and run all the tests with `-v` (to print them out). You can actually do this in the travis build to (edit `ci/script.py` to put in a `-v` in the `NOSE_ARGS` so that you can see where it is hanging.

I'm not sure I understand how `ci/script.sh` works (there is no `ci/script.py`) -- I tried changing `"$NOSE_ARGS"` to `"$NOSE_ARGS -v"` but it didn't work. Where should I add the arg `-v`?

try this: https://github.com/jreback/pandas/commit/f7f4d6e2776cf9e0776645cc1834cbdd52bf5daa

@jreback - Uggh, I figured out the problem, and it looks like it has to do with how the Python parser handles rows...it turns out that Travis was halting on a call to `read_html`, which ultimately boiled down to something like the call `PythonParser([[], [], []], header=0, index_col=0)` (which gets stuck in an infinite loop). I adjusted `_next_line()` to increment `self.pos` within the loop, but then I ran into an issue with a multi-index header and discovered that the parser is using `self.pos` synonymously with the current row index...I think we'll need a new variable to monitor the actual row index, maybe `self.row_pos`. I'll try to fix this in the next couple days. 

ok great!

Ok what I have now is looking better, but there's a point of ambiguity with the parameter `skiprows`. Should this continue to work with actual line numbers, or should it discount commented/empty lines? e.g. with this data:

```
1 2 3

4 5 6
```

should `skiprows` treat `4 5 6` as line 2 or line 1? (0-based indexing)

I think skiprows should be based on actual rows