BUG/MAINT: Change default of inplace to False in pd.eval
State: closed

Deprecated back in 0.18.0.

xref #11149.

Also patches bug where we were improperly handling the `inplace=False` condition, as we were assuming that target input was non-None when that wasn't necessarily enforced.


Labels:
name: API Design
name: Deprecate
description: Functionality to remove in pandas

Comments:
Apparently, >200 (!) of our tests didn't take heed of the warning to pass in `inplace=True`
Also, I must say that the docstring for `inplace` is not very intuitive IMO.  I would have expected that `inplace=False` would return the result of the computation and not the other way around.

@jreback @jorisvandenbossche : Thoughts?
Whoops, sorry, all my comments were in light of me thinking it was about `DataFrame.eval` method, not top-level `pandas.eval`. 

Anyhow, the frame method also has to be updated, and there the comments will be sensible :-) (edit: but actually the docstring there is already much clearer!)
@jorisvandenbossche : Actually ignore my previous comment.  That doesn't make much sense to me at all still.  The documentation says "whether to return a new DataFrame or mutate the existing."  Which condition means return a new `DataFrame`?

If you say it's `inplace=False`, then why are we returning `target` in `pd.eval` (and not `ret`)?  Also, why are we returning anything when `inplace=True` in `pd.eval`?

IMO, it should be that `ret` is returned when `inplace=False`, and nothing is returned when `inplace=True` because `target` should be modified.  In fact, I might go as far as to say that `inplace=True` only when `target` is not a primitive (and raise otherwise).  Otherwise, not really sure what this means here:

~~~python
>>> import pandas as pd
>>> c = pd.eval('1 + 2', inplace=True)
>>> c
3
>>> c = pd.eval('1 + 2', inplace=False)
>>> c
>>>
~~~
cc @chris-b1 
My thought is that `inplace` should only be part of the api for `DataFrame.eval` - there it works as expected, and consistent with other uses of `inplace`.  For `pd.eval` it is simply an implementation detail (since `DataFrame.eval` calls it) - I should not have added it to the docstring.

In fact, `pd.eval` should probably raise if `inplace` is passed in (so keep the default = None) without a `target`.

@chris-b1 : I think we are mostly aligned in our thought process, and I think (if nobody objects) that we should address the issue now in this PR instead of waiting.

I don't think adding `inplace` to the doc-string was a bad idea.  It's just that I don't understand why it behaves so counter-intuitively to what I would think `inplace` does in `pd.eval`.  Why is it that `target` is returned when `inplace=False`?  It should be the other way around.

I agree that we should only allow `inplace=True` when `target` is not a primitive.  Furthermore, we should be returning anything when `inplace=True` because we assume that `target` itself has been modified.

What do you think?
The question we should ask: what is the usecase of `inplace`? (is there actually a usecase?) 
I don't see one, so I agree with @chris-b1 we better remove it from the docstring / document it as no effect.

Possible usecases:

* expression without assignment:
  * this should always return something, as there is no object to return to (passing a target also, logically, does not do anything with the target). So `inplace` makes no sense in this case.
  * The reason that `inplace=False` actually suppresses the return value is just a bug in the current implementation. The code does:

    ```
    if not inplace and inplace is not None:
        return target

    return ret
    ```

    which fails for inplace=False without a target.

* expression with assignment:
  * for this case, you are obliged to pass a `target`, so why would you not want to modify this target?
  * That's my first thought, but OK, here might indeed be a possible usecase: eg you want to add a column to an existing dataframe, but still want to take a copy of it first, and then return the modified result instead of modifying inplace. Similar as we do with eg `assign`

So actions I would take:

- Fix the bug with inplace in case of no assignment. And I would even additionally raise an error if `inplace` is specified in such a case
- Update the docstring to make it clear that `inplace` is only relevant for the case where a `target` is passed.
@jorisvandenbossche : Well that touches upon #16529, but in any case, it can be used when `target` is a `DataFrame` object, so I wouldn't say that it has no use case at this point.

I agree that we should update the docstring to say that `inplace` has no meaning when `target` isn't a non-primitive object (passing in `target=1` also makes no sense).

Also, the fix for that bug *should be* the following:
~~~python
<if target is None or is some primitive>:
   inplace = False
...
if not inplace:
   return ret
~~~

There should be no condition for returning anything if `inplace=True` because we assume that `target` itself is modified inplace.

How does that sound?  @chris-b1 thoughts?
I would suggest

 - `DataFrame.eval`, change `inplace` default to `False`
 - `pd.eval`

    - keep default for `inplace` as `None`
    - if `inplace` kw is passed with no target, raise
    

@chris-b1 : I agree with `inplace=False`, but I don't agree with your suggestion about `inplace=None`.  The only time I would consider raising is when `inplace=True` (`inplace=False` shouldn't care about `target`) and `target` is something that you can't modify in-place.  Also, I think we should default `inplace=False` to `pd.eval`, as that is consistent with what we have for other `inplace` defaults.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=h1) Report
> Merging [#16732](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/329fdaae294ae3dc45d4d7301f0f1eec2d26cb4b?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #16732      +/-   ##
==========================================
+ Coverage   90.94%   90.97%   +0.02%     
==========================================
  Files         161      161              
  Lines       49272    49283      +11     
==========================================
+ Hits        44811    44834      +23     
+ Misses       4461     4449      -12
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.74% <100%> (+0.02%)` | :arrow_up: |
| #single | `40.16% <5%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/computation/eval.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tcHV0YXRpb24vZXZhbC5weQ==) | `96.93% <100%> (+2.37%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.71% <100%> (+0.04%)` | :arrow_up: |
| [pandas/io/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2lvL2NvbW1vbi5weQ==) | `67.79% <0%> (-2.12%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `95.8% <0%> (-0.01%)` | :arrow_down: |
| [pandas/core/window.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvd2luZG93LnB5) | `96.49% <0%> (ø)` | :arrow_up: |
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.07% <0%> (ø)` | :arrow_up: |
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `96.65% <0%> (ø)` | :arrow_up: |
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `94.79% <0%> (+0.04%)` | :arrow_up: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `90.58% <0%> (+0.19%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16732?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/16732?src=pr&el=footer). Last update [329fdaa...c6f5bcd](https://codecov.io/gh/pandas-dev/pandas/pull/16732?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jreback @jorisvandenbossche : Could you run the failing build on Travis?  It doesn't seem related to my changes at all whatsoever.
Thanks to whoever reran the build!  Everything is green now and ready for review.
I don't really recall why there is a ``target`` object for assignment here at all. maybe @cpcloud remembers.

This seems odd to have ``target`` AND ``inplace``. Since inplace is our standard, would deprecate ``target``, default ``inplace=False``
`target` is on `pd.eval` because `DataFrame.eval` calls it internally, and it is used to set the scope to calling df.
> target is on pd.eval because DataFrame.eval calls it internally, and it is used to set the scope to calling df.

so then either pass a private arg ``_target``, then this is no problem. or do a wrapper method that ``DataFrame.eval`` calls (that handles the ``target`` stuff) rather than the public ``eval`` (maybe a bit more work, but prob 'cleaner')
> so then either pass a private arg _target, then this is no problem. or do a wrapper method that DataFrame.eval calls

Ummm, `target` is pretty ingrained in the code.  I think the "cleanest" is to enforce `target` being a `DataFrame` (similar to what we do with `pd.merge` for example) and change the signature as follows:

~~~python
def eval(target, parser=...):
~~~

However, you MUST then have item assignment if this is the case for it to make any sense.  I don't know how many people use `pd.eval` (and what the use-case is beyond `DataFrame`).
@gfyoung not sure what you mean the ``target`` argument itself is almost the last kwarg.
@jreback : I'm saying that we should change the signature to place the `target` as the first-class citizen in this function by putting it at the start of the signature just like we do in `pd.merge` for example.  That would help to clear up a lot of confusion regarding the specifications surrounding `pd.eval`.

In addition, we should require that `target` is a `DataFrame` (just like we do in `pd.merge`) as well as enforce that each line of the expressions have item assignment for this to make any sense (though I don't know how much `pd.eval` is used beyond this context of `DataFrame` item assignment).
> @jreback : I'm saying that we should change the signature to place the target as the first-class citizen in this function by putting it at the start of the signature just like we do in pd.merge for example. That would help to clear up a lot of confusion regarding the specifications surrounding pd.eval.

seems rather extreme change for limited upside. 

> In addition, we should require that target is a DataFrame (just like we do in pd.merge) as well as enforce that each line of the expressions have item assignment for this to make any sense (though I don't know how much pd.eval is used beyond this context of DataFrame item assignment).

yes
> seems rather extreme change for limited upside.

Really?  It's literally just moving the position of the parameter to indicate importance:

~~~python
def eval(target, expr,...)
~~~
> Really?

That breaks all user code that uses `eval`, so indeed rather invasive?
I don't think we need to change anything about `target`. It's probably not something a lot of people use or find useful, but it is there for internal use and for the power user, so just lets leave it there. 


> That breaks all user code that uses eval, so indeed rather invasive?

Right, but as this is for a major version release, an API change like this isn't too drastic, is it?

> It's probably not something a lot of people use or find useful, but it is there for internal use and for the power user, so just lets leave it there.

That's fair.  As the implementation stands, I don't mind leaving it there.  However, I do think we will want to consider restricting it to just `DataFrame`, in which it should move up in the signature.
> Right, but as this is for a major version release, an API change like this isn't too drastic, is it?

Well, even for a major release we don't just want to break something, it has to have a good reason. And I don't think there is a good reason here. And more importantly, it is even an optional argument, so IMO that makes no sense to put it first.

>  However, I do think we will want to consider restricting it to just DataFrame, in which it should move up in the signature.

Why is moving needed? I don't see any relation with restricting what you can pass to `target` and the need to move `target` in the signature
So judging from the conversation, it seems like any drastic changes to `pd.eval` in terms of aligning it with other top-level functions `pd.merge` are probably not going to happen.

However, I'm okay with leaving the implementation as is (and not verify `target`).
@jorisvandenbossche (in response to your comment) : I was proposing that we could drastically modify `pd.eval` to operate just on `DataFrame` (so require `target`), which is why we would move it up on the signature.  This is to align the behavior and signature with other top-level functions like `pd.merge`, which operate solely on `DataFrame`.

However, I'm okay with leaving this discussion for a subsequent PR (if necessary).
I just don't see any argument for such a drastic change.

> I was proposing that we could drastically modify pd.eval to operate just on DataFrame (so require target), 

Ah, OK, then it makes some sense. But, for that you can already use `DataFrame.eval`. Further, that defeats the purpose of `eval` to just execute the expression without a dataframe as the context (it can be used for that, so probably people do it). 
It is true that now the equivalence between `pd.eval` and `DataFrame.eval` is not as consistent as eg with `merge` (pd.merge(df, ..) is indeed just df.merge(..)), but I don't think it is worth it for `eval` to change this.

@jorisvandenbossche : Fair enough.  I think this PR does enough as it is, hence why I'm fine with leaving this discussion for another time.