API: Deprecate skip_footer in read_csv
State: closed

Title is self-explanatory.

Closes gh-13349 and partially undoes this <a href="https://github.com/pydata/pandas/commit/9ceea2fd46e79f37269a32de8c140cddf90eda13">commit</a> back in `v0.9.0`.  With such a massive API now, having duplicate arguments makes managing it way less practical.


Labels:
name: API Design
name: IO CSV
description: read_csv, to_csv
name: Needs Discussion
description: Requires discussion from core team before further action

Comments:
In light of removing duplicate arguments and stripping down the API a bit, here are a couple of others I would like to propose (perhaps not in this PR but for discussion nonetheless):

1) Deprecate `skipfooter` in `read_excel` (it's allowed as an alternative to `skip_footer`, though it is not surfaced in the signature but rather encompassed in a `**kwargs` argument)?

2) Should we choose between `sep` and `delimiter` in `read_csv`?

the problem is that we also have `skiprows`. I would actually deprecate `skip_footer` instead.

Hmmm...that's a flip-flop AFAICT from what you said earlier <a href="https://github.com/pydata/pandas/issues/13349#issuecomment-223450898">here</a>.  However, deprecating `skip_footer` seems a little strange since we use `skip_footer` all over the place in the code (internally especially).  That's why I chose to deprecate `skipfooter`, since the code impact is not very significant as you can tell.  In addition, we use the `_` in a lot of the other arguments, so I'd rather side with the majority.

just change it internally to the correct kw. I think they diverged at some point. It looks (just a quick skim), that `skipfooter` is more consistent (as we have `skiprows`, `usecols`) etc. excel should be fixed to be the same as well.

@jreback : let's actually do the count of **multi-word argument names** (excluding the `skip(_)footer` ones) using the documentation as of `0.18.1` <a href="http://pandas.pydata.org/pandas-docs/stable/generated/pandas.read_csv.html">here</a>:

Arguments that use `_`:
`filepath_or_buffer`
`index_col`
`mangle_dupe_cols`
`true_values`
`false_values`
`na_values`
`keep_default_na`
`na_filter`
`skip_blank_lines`
`error_bad_lines`
`warn_bad_lines`
`delim_whitespace`
`as_recarray`
`compact_ints`
`use_unsigned`
`low_memory`
`buffer_lines`
`memory_map`
`float_precision`

Arguments that don't use `_`:
`usecols`
`skipinitialspace`
`skiprows`
`chunksize`
`lineterminator`
`doublequote`
`quotechar`
`escapechar`

Even excluding ones that have been recently deprecated (`compact_ints`, `buffer_lines`, `use_unsigned`, `as_recarray`), arguments that use `_` vastly outnumber those that don't.

@gfyoung I am trying to minimize back-compat pain.

`usecols` and `skiprows` are prob some of the most used options. I agree we should just deprecate and move to `_` separated, but that would be in 0.19.0 if we go whole hog.

## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/13386?src=pr) is 85.23% (diff: 100%)

> Merging [#13386](https://codecov.io/gh/pydata/pandas/pull/13386?src=pr) into [master](https://codecov.io/gh/pydata/pandas/branch/master?src=pr) will increase coverage by **<.01%**

``` diff
@@             master     #13386   diff @@
==========================================
  Files           140        140          
  Lines         50420      50419     -1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          42975      42975          
+ Misses         7445       7444     -1   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [cc216ad...d21345f](https://codecov.io/gh/pydata/pandas/compare/cc216ada82a2b06a52524da7b1559a7f5e31b1c1...d21345f3536437713062da0f55bcc00fc4877476?src=pr)

@jreback : I have no issues waiting until `0.19.0` and going full hog.  This duplicate argument is not breaking, and it will then give us the time to properly deprecate and rename arguments with `_`.

Rebased onto `master` and Travis is passing.  I see that @jorisvandenbossche has added the "Needs Discussion" tag, so comments on the deprecation and of what to deprecate are welcome!

this looks good to me. need to have a nice way of communicating the deprecations in the doc-string. typically we add an additional entry for the new one and make the original kwarg have a DEPRECATED in bold. Here we need to do something different as so many args.

maybe just list the new ones, but have a sentence that says the orginal arg is deprecated. 

AND have a section before the arguments start that says, hey all of these are deprecated.

finally, need to create an issue (maybe just a single one), to have `read_excel`, `read_html`, `read_hdf` made consistent with this (I don't think any others, but should check). e.g. `chunksize` for sure is one. 

also have issue #4988 which is kind of what I am suggesting for above (e.g. its about `read_excel`); so maybe repurpose that one to be the non-csv-parser but same kw-in-need-of-deprecation issue.

@jreback : Agreed about `read_excel` (and the others that you mentioned) - that has a few arguments that could be changed, though best saved for another PR.  I was planning on looking those afterwards after working out the kinks with this one here.  The ones to check I think are:

`read_excel`
`read_html`
`read_sql`
`read_hdf`
`read_stata`

The reason I added "Needs discussion" is because I am not sure if I find this a good idea. `read_csv` is one of the most used pandas functions, and causing thousands/millions (?) users to have to change their code just for 'aesthetic'\* reasons, not sure this is worth the trouble.

\* I know this is not _only_ aesthetic, because having a consistent API (eg always underscores) also makes it easier to learn, to remember, etc .. And that is very important, but sometimes 'historical' reasons are good reasons.

On the specific keywords:
- `dayfirst` is also used in `to_datetime`, `Timestamp`, and comes from `dateutil.parser.parse` , so I would leave this is as.
- `chunksize` is also used a lot in other places (`read_hdf`, `read_sql`, `to_sql`, ..), so I would also be more hesitant to change this

an alternative is simply to accept aliases, e.g. `skiprows` & `skip_rows`. This only adds a very tiny amount of code (in getting kwargs). and 'solves' the problem pretty easily. 

Yes, you could see it as a "deprecation by documentation": documenting the recommended way, but not actually deprecating (and maybe a small note the docstring that certain list of keywords are also accepted for historical reasons). 
This options of course also has some overhead and disadvantages (possible confusion when users see code using the 'old' args)

@jorisvandenbossche :

`chunksize` is one that @jreback and I discussed earlier in the conversation as converting to `chunk_size` (note the list that I made <a href="https://github.com/pydata/pandas/pull/13386#issuecomment-231613825">here</a>).

As for `dayfirst`, I do realize that there are some inconsistencies in deprecating it, especially since `datetime` uses that parameter.  However, a consistent API is useful (i.e. all underscores), so similar to what @jreback is suggesting, we could just document only the recommended parameters but still accept the old parameters with `DeprecationWarning` and message about the recommended parameters (as I do now).  However, I can add a `TODO` or comment saying that we should NOT remove support for the old parameters for some time.

How does that sound?

@gfyoung nice table!

but I agree with @jorisvandenbossche I don't think we should deprecate these keywords. It really not a big deal to simply accept both in the constructors (and document it in the doc-strings).

There aren't that many. What I mean is change the `pd.read_csv` to accept `line_terminator`, but also allow `lineterminator` (you will have to do a check that _both_ are not defined), but this can be automated. I DO think that we should change all internal code / docs to use the underscore separated ones (as you have done). So most of your docs can stay. 

I just don't think we should actually deprecated them (but instead deprecate in the future) after this has been out there for a bit.

kind of like a PendingDeprecationWarning.

@jreback : Thanks!  It's amazing what you can do when all you have to do is copy. :smile: 

I don't have issues with accepting both.  In fact, my PR will continue to accept both representations (with or without underscores).  I only suggested deprecating the underscore-less ones because I've been through this on the `numpy` side when I deprecated `random_integers` but the plan wasn't to remove it for the foreseeable future.  I guess it depends on how you interpret deprecation.

well, let's not get started on what `numpy` does or doesn't do :)

Well technically, it's what I did, not `numpy`. ;)  Well, if you insist, I use `PendingDeprecationWarning` instead of `DeprecationWarning` then.

 I would just have an issue that schedules us to do it. though not really against `PendingDeprecationWarning`

Hmmm...given that it's such a massive API change, I wouldn't mind warning users in advance of the deprecation.  Keep in mind that we might be extending this to other I/O methods too.

yeah I guess that is appropriate, even python itself uses them: https://github.com/numpy/numpydoc/issues/35

@jorisvandenbossche ?

@jreback : <a href="https://github.com/pydata/pandas/pull/13386#issuecomment-232235928">hypocrite</a>! :smile:

> but I agree with @jorisvandenbossche I don't think we should deprecate these keywords. It really not a big deal to simply accept both in the constructors (and document it in the doc-strings).

Yes that is how I see it as well.

> kind of like a PendingDeprecationWarning.

So then just let's use one :-) But the questions is maybe: are we ever going to actually deprecate them? If not, we maybe don't need to use any warning (but also in that case you could see the PendingDeprecationWarning as a kind of NotRecommendedKeywordPleaseUseOtherWarning :-), so maybe no problem with having that). 
In any case, I would not use a FutureWarning

> As for `dayfirst`, I do realize that there are some inconsistencies in deprecating it, especially since datetime uses that parameter. However, a consistent API is useful.

-1 on changing the `dayfirst` kwarg. An inconsistent naming scheme within a function is not good, but having inconsistent names for the same thing between functions is even worse IMO. So either we should rename it everywhere, or either keep it here as `dayfirst`. 
But personally my preference is just to keep it as `dayfirst` (given that it corresponds with a dateutil kwarg). Having one keyword that deviates from the naming scheme is not that a big deal I think.

> > `chunksize` is also used a lot in other places (`read_hdf`, `read_sql`, `to_sql`, ..), so I would also be more hesitant to change this
> 
> chunksize is one that @jreback and I discussed earlier in the conversation as converting to chunk_size.

Maybe it is my Dutch mother tongue that I find `chunksize` good as one word :-) (in Dutch, such a compound word would be one word instead of two separate words)

Since this is a rather big change (regarding user impact), I think it is good to have some more eyes on it. cc @TomAugspurger @sinhrks @shoyer @wesm @chris-b1 If you have any objections/opinions on the general idea, time to raise them.

I would say leave chunksize and dayfirst alone as joris indicated

I would lean against this change -- I don't think deprecating or even adding aliases with underscores for these keywords is worth it. Yes, consistency is nice, but these are keywords that are mostly either (a) copied straight from the API docs or an example or (b) auto-completed. Omitting underscores between lower case words is also common enough in PEP8 compliant code that it doesn't hurt my eyes in the way that camelCase would, for example. I would save changes like this for next time somebody writes a DataFrame library ;). There's just very little upside to the change at this point.

I'd also be inclined to not do this.

Consistency is great, but to me, big breaking (or warning triggering) changes for consistency are only worth it to the extent they actually improve the user experience.  E.g. `resample`, the new `sort_...` api, or  a closer example, when the inconsistency between the `rows` and `index` keyword was resolved in `pivot_table` / `pivot` - #5505

This doesn't seem to meet that hurdle.  Like @shoyer said, these argument would be typically tab-completed or looked up, or even if not, the inconsistency doesn't cause any confusion.

To put things in perspective, this whole discussion was generated because I wanted to remove the duplicate 'skipfooter/skip_footer' argument in the signature.  It really should only be one of the two IMO.

The consistency part came up because we weren't sure which way to go i.e do we take the underscore version or the non-underscore?

I perfectly understand if we don't want to make such a change as drastic as it has become, though some input on the initial goal would be good then.