DEPR: Warn about Series.to_csv signature alignment
State: closed

Warns about aligning `Series.to_csv`'s signature with that of `DataFrame.to_csv`'s.

In anticipation, we have moved `DataFrame.to_csv` to `generic.py` so that we can later delete the
`Series.to_csv` implementation, and allow it to adopt `DataFrame`'s `to_csv` due to inheritance.

Closes #19745.

cc @dahlbaek

Labels:
name: Output-Formatting
description: __repr__ of pandas objects, to_string
name: API Design
name: IO CSV
description: read_csv, to_csv
name: Deprecate
description: Functionality to remove in pandas

Comments:
cc @toobaz @jorisvandenbossche 
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21868?src=pr&el=h1) Report
> Merging [#21868](https://codecov.io/gh/pandas-dev/pandas/pull/21868?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/365eac4d27353693a2b758c87fc4bceabb74817f?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/21868/graphs/tree.svg?src=pr&token=eZ4WkYLtcO&width=650&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/21868?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #21868      +/-   ##
==========================================
+ Coverage   91.91%   91.91%   +<.01%     
==========================================
  Files         164      164              
  Lines       49992    50008      +16     
==========================================
+ Hits        45951    45967      +16     
  Misses       4041     4041
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.3% <100%> (ø)` | :arrow_up: |
| #single | `42.15% <39.28%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21868?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/21868/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.18% <ø> (-0.02%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/21868/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.47% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/21868/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.18% <100%> (+0.07%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21868?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21868?src=pr&el=footer). Last update [365eac4...38aebd0](https://codecov.io/gh/pandas-dev/pandas/pull/21868?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

My understanding from #19745 was that the differences were limited (``path``, ordering, ``header`` default value) and that hence we could have raised warnings only for those, and specific errors for them.

Couldn't we change the first argument to ``path_or_buf``, append the ``path`` argument at the end of the signature, and then emit the warning (and assign its value to ``path_or_buf``) if ``path is not None``?

Similarly, couldn't we set the default value of ``header`` to None, and translate it to ``False`` (if it is ``None``) but emitting the ``FutureWarning``?

I know we would still not catch the ordering change, but
- probably the large majority of users will get a warning from ``header`` - and hence they will be informed about the signature change
- I expect very very few users to pass ``index=`` (which is the first argument after ``buf``) as a positional argument

Apart from the comment above, my main problem with this PR is that it seems to enforce (at least if one wants to suppress the warning) passing ``path`` as a named arg, which I think is unnatural and unneeded.

@toobaz : The original PR IMO is only a band-aid to the larger issue that `Series.to_csv` is not in sync with `DataFrame.to_csv`.  That's why I'm going all in and issuing a warning as is.  A major release (`0.24.0`) is the best time to do this.

If your main issue with this PR is that people need to pass in a keyword arg for `path`, I'm okay with that "inconvenience", especially since `DataFrame.to_csv` is a much more commonly used IMO than `Series.to_csv`.  Also, it will certainly make people aware of the fact that we are going to switch signatures in the future.

A single keyword inconvenience is worth it for all of the keyword arguments and awkwardness that we will resolve with the fact that the functions are not synchronized, both for end users and us as maintainers.
Regarding silencing the warning, perhaps one could add to the warning message that it can be silenced by explicitly casting the `Series` instance `ser` to `DataFrame` using `DataFrame(ser)`? As far as i can tell, this would be a future-proof method (i.e. `to_csv` is implemented on `Series` by first casting to `DataFrame` and then calling `to_csv` on the resulting `DataFrame`). In that case, one could even go to the extreme and simply always show the warning message when `Series.to_csv` is called.
> Regarding silencing the warning, perhaps one could add to the warning message that it can be silenced by explicitly casting the Series instance ser to DataFrame using DataFrame(ser)?

We might as well state that we will completely remove ``Series.to_csv``! I don't see any reason for trying to change the way users use a function which works just fine.
> especially since DataFrame.to_csv is a much more commonly used IMO than Series.to_csv

Again, having a seldomly used function is not a good argument, to me at least, to make it more annoying to use.
> We might as well state that we will completely remove `Series.to_csv`! I don't see any reason for trying to change the way users use a function which works just fine.

Removing `Series.to_csv` is not the same as changing the signature, so stating that `Series.to_csv` will be removed would be false information. No matter what you do, the user will be bothered by a signature change. This way, the user has three options:

1) Live with the warnings and do not change the arguments. Result: Warnings, not future-proof.
2) Change the arguments, and live with the warnings. Results: Warnings, future-proof.
3) Avoid warnings by not using the "unstable" method (i.e. explicitly recast to `DataFrame`). Result: No warnings, future-proof.

Note that the third option is simply the user implementing the fix by hand, which is why it is future-proof and does not emit warnings. I think those all seem like reasonable options for a short deprecation period.
So let me meet you guys half-way:

* Don't warn on the `path` argument
* Change `header` to default to `None`
* Warn if `header` isn't explicitly set

How does that sound?
> So let me meet you guys half-way:
> 
>     Don't warn on the path argument
>     Change header to default to None
>     Warn if header isn't explicitly set
> 
> How does that sound?

I think that's reasonable... I just think that moving ``path`` as last argument, and warning if ``path is not None``, is superior (because those people who passed ``path=`` will get the warning): can you point at any downside?
> I just think that moving path as last argument, and warning if path is not None, is superior (because those people who passed path= will get the warning): can you point at any downside?

@toobaz : I'm not sure I fully understand you here.  What do you mean by "moving path as last argument" ?  It sounds like you're just repeating what I did currently in this PR?
> I think those all seem like reasonable options for a short deprecation period.

My point is: if a given thing works now _and_ will work in the future _in the same way_, no deprecation period should force users to do things otherwise.

In this case, the "thing" is "passing a filename as first positional argument", which (correct me if I'm wrong) works exactly as it should, and will.

Contrast this with the warning for ``header=``, which will rightly force people to change their calls _because the behavior of that argument_ will change.
> @toobaz : I'm not sure I fully understand you here. What do you mean by "moving path as last argument" ?

My idea is that ``Series.to_csv`` could look like:

```python
def to_csv(self, path_or_buf=None,
           [rest of signature],
           path=None)
if path is not None:
    # emit warning about change of argument name
    assert(path_or_buf is None), "You cannot pass both 'path' and 'path_or_buf'."
    path_or_buf = path
[...]
```

where ``[rest of signature]`` includes all other arguments, _in the order they appear in ``DataFrame.to_csv()``_.

This will be complemented with the warning for ``header=`` if unset and (this just came to my mind now) with

```python
if isinstance(sep, bool):
    # emit warning about the change of arguments ordering
    raise ValueError
```

This way, we inform user of the old ``Series.to_csv`` positional arguments that the signature ordering has changed (previously, ``index`` was the first positional argument after ``path``, now it is ``sep``).
The only downside (I can see) is that for those users, there will be immediate breakage, rather than a warning... but again, I expect it to be a really rare use case. This downside _could_ be avoided by reordering all arguments (only inside this ``if`` branch)... it's maybe just not worth the effort.
> My idea is that Series.to_csv could look like:

Hmm...I see...IMO that's bending over backwards a little too much duplicating (and bloating the signature) like that.  I would leave it at warning regarding the `header` argument.
> that's bending over backwards a little too much duplicating (and bloating the signature) like that

Just to be sure we are on the same page: it would obviously be a temporary measure, ``drop=`` would be removed at the end of the deprecation period.
> Just to be sure we are on the same page: it would obviously be a temporary measure

@toobaz : Oh, I definitely understand. 🙂 My comment [above](https://github.com/pandas-dev/pandas/pull/21868#issuecomment-404668190) was made with that consideration in mind.
> @toobaz : Oh, I definitely understand. slightly_smiling_face My comment above was made with that consideration in mind.

OK. Would your comment change if I proposed the following rather than the above?

```
def to_csv(self, path_or_buf=None,
           [rest of signature],
           **kwargs)
if kwargs.get('path', None) is not None:
    # emit warning about change of argument name
# continue as in your PR
```

Concerning the possibility that ``[rest of signature]`` follows the ``DataFrame.to_csv`` order: this is orthogonal with respect to the discussion on whether to warn for ``path=``. And my reasoning is that if we do _not_ change the signature order now, people might have to change their calls twice: now to avoid the warnings, and later to conform the signature. This unless they refrain from using positional arguments at all - which is probably good style, but which we probably don't want to impose.
> OK. Would your comment change if I proposed the following rather than the above?

@toobaz : Ah...that I think we can do.  Nice!
If we're not emitting a catch-all warning, why not go all the way with a custom-made decorator instead? Something which emits a warning if `header` is not passed, or there are more than 2 positional arguments, or if `path` is passed? I'm not very familiar with how the warning and deprecation systems work in the pandas code base, but here's a rough sketch of how it could be implemented:

```python
from functools import wraps
def deprecation_decoration(func):
    @wraps(func)
    def _func(*args, **kwargs):
        if len(args) > 2:
            print("The signature of `Series.to_csv` will be "
                  "aligned to that of `DataFrame.to_csv` in the "
                  "future. Note that the ordering of positional "
                  "arguments is different, so please refer "
                  "to the documentation for `DataFrame.to_csv` when "
                  "changing your function calls. To ensure "
                  "future-proof calls, do not pass more than 1 "
                  "positional argument.")
        if "path" in kwargs:
            assert len(args) <= 1, ("Cannot specify path both as positional "
                                    "argument and keyword argument.")
            assert "path_or_buf" not in kwargs, ("Can only specify path "
                                                 "or path_or_buf, not both.")
            print("FutureWarning: path is deprecated, use path_or_buf instead")
            kwargs["path_or_buf"] = kwargs.pop("path")
        if "header" not in kwargs:
            print("The signature of `Series.to_csv` will be "
                  "aligned to that of `DataFrame.to_csv` in the "
                  "future. The default value of header will change. "
                  "To keep the current behaviour, use header=False.")
        return func(*args, **kwargs)
    return _func
class my_class:
    def __init__(self):
        pass
    @deprecation_decoration
    def my_func(self, path_or_buf=None, index=True, sep=";", header=False):
        pass
```
@dahlbaek : IMO that's a little too much code for this deprecation.  When deprecating, we want to keep it as simple as possible.  Decorators add a level of indirection that is not necessary.
In that case, how about testing for positional arguments beyond `path_or_buf` by changing the signature like this

```python
def to_csv(self, path_or_buf=None, *args, index=True, sep=",", na_rep='',
    float_format=None, header=None, index_label=None,
    mode='w', encoding=None, compression=None, date_format=None,
    decimal='.', **kwargs):
```

and testing for positional arguments beyond `path_or_buf`

```python
if args:
    warnings.warn("The signature of `Series.to_csv` will be "
                  "aligned to that of `DataFrame.to_csv` in the "
                  "future. Note that some of the default arguments "
                  "and argument names are different, so please refer "
                  "to the documentation for `DataFrame.to_csv` when "
                  "changing your function calls.",
                  FutureWarning, stacklevel=2)
    raise Exception("Pass at most 1 positional argument. Further arguments should be passed as keyword arguments.")
```

I would rather have my code crash now, with an error message, than have it silently change its behaviour in the future.
> IMO that's a little too much code for this deprecation.

To be fair, I think it is less code, because the signature is not repeated, and most of it is just messages for the user. Note also that it includes an extra test: It tests if there are enough positional arguments that the behaviour will change due to reordering of the arguments when the signatures are aligned.

Fair point concerning the extra level of indirection though.
> In that case, how about testing for positional arguments beyond path_or_buf by changing the signature like this

Definitely don't want to raise an `Exception` for a deprecation.  Also, throwing `*args` in the middle of a signature is generally more confusing to the end user IMO.

> I would rather have my code crash now, with an error message, than have it silently change its behaviour in the future.

It isn't silently changing though.  In fact ATM, we're warning on the things (`path_or_buf` instead of `path` and the default argument for `header`) that would break backwards compatibility.  If you aren't triggering the warning, then your code will be forwards-compatible.
> raise Exception("Pass at most 1 positional argument. Further arguments should be passed as keyword arguments.")

Again: we don't want to forbid behaviors which are legitimate now _and_ in the future.
> Decorators add a level of indirection that is not necessary.

I do think that decorator proposal is overly complicated (and again: if we want to warn for the changing order for positional arguments, we can just check the type of the second positional argument), but a self-contained deprecation decorator has the huge bonus of being very simple to remove (when the deprecation period is over).
> but a self-contained deprecation decorator has the huge bonus of being very simple to remove (when the deprecation period is over).

@toobaz : That's moot here though.  When the deprecation is over, we're deleting the entire function regardless and allowing `Series` to inherit from `NDFrame`. 🙂
OK, trying to recap:
- [x]  decorator: no
- [x]  warning for ``header=``: yes (defaults to ``None``)
- [x]  warning for ``path=``: yes, only [if passed as keyword argument, by checking](https://github.com/pandas-dev/pandas/pull/21868#issuecomment-404687302) ``**kwargs``
- [ ] ordering of arguments: I suggest to move to the ``DataFrame.to_csv`` one, other preferences?
- [ ] warning on old arguments order: I suggest (weak preference) to check the dtype of the second positional argument, other preferences?
- [ ] raising on old arguments order: I would prefer not, but it's acceptable as long as we do check the dtype of the second positional argument (and do not just raise on positional arguments)
@toobaz : Argument ordering checking is going to be very messy because there are going to be a good number of parameters to check.

Also, some of them are not particularly easy to distinguish.  Notice how the position of `sep` in `Series.to_csv` corresponds to the position of `na_rep` in `DataFrame.to_csv`.  Both are strings.

Thus, for the reasons above (and partially because of the work itself involved not going to lie 🙂 ), my preference is to leave the signature alone and avoid the mess of argument order / type validation.
> Argument ordering checking is going to be very messy because there are going to be a good number of parameters to check.

As I said above: I would only check the second. You can't pass the third without passing the second ;-)

_Reordering_ them is more messy.
> As I said above: I would only check the second.

@toobaz : Ah, oops, I overlooked that detail when reading.  Hmm...I'm not a big of that one just because a simple command like `Series.to_csv(path)` will issue a warning (unnecessarily).