Fixed 'add_methods' when the 'select' argument is specified.
State: closed

The 'add_methods' function was not behaving correctly when the 'select' argument was specified. This case was not covered by tests - https://codecov.io/gh/pandas-dev/pandas/src/master/pandas/core/ops.py#L149 .
The function is not fully tested and it is unclear how to write a proper test for it. The function is currently not used with the 'select' argument in any place within pandas. The intent of the code was clear and the bug was obvious by plain reading.

Labels:
name: Bug
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17045?src=pr&el=h1) Report
> Merging [#17045](https://codecov.io/gh/pandas-dev/pandas/pull/17045?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/4efe6560e07f28de6a1834fa90e31cef31b0fb18?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `0%`.

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

```diff
@@            Coverage Diff             @@
##           master   #17045      +/-   ##
==========================================
- Coverage   91.05%   91.03%   -0.03%     
==========================================
  Files         161      161              
  Lines       49350    49350              
==========================================
- Hits        44936    44926      -10     
- Misses       4414     4424      +10
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.81% <0%> (-0.01%)` | :arrow_down: |
| #single | `40.26% <0%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17045?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/17045?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `91.86% <0%> (-0.17%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17045?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17045?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.1%)` | :arrow_down: |

------

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

@HonzaOsobne : Thanks for the PR!  For this to be considered a bug, you should be able to provide an example that surfaces this bug.  Otherwise, it's hard for us to evaluate and (prevent in the future) such an issue from happening.

Too bad that being visually obvious can't be tested on Travis :smile: 
Hello @gfyoung , an example is provided below (Python 2.7.13):

```
import pandas as pd

def _enable_import_of_the_ops_module_even_if_it_is_private():
    import os
    import sys
    sys.path.append(os.path.join(os.path.dirname(pd.__file__), "core"))

_enable_import_of_the_ops_module_even_if_it_is_private()
import ops


class MockSeries(object):
    pass


keys_original = set(MockSeries.__dict__.keys())

# 'select' is supposed to be a whitelist, the 'ops.series_flex_funcs' contains many more keys
ops.add_flex_arithmetic_methods(MockSeries, select=("gt",), **ops.series_flex_funcs)

keys_after_addition = set(MockSeries.__dict__.keys())
keys_added = keys_after_addition - keys_original

assert len(keys_added) == 1, \
    ("Only the 'gt' method was expected to be added, "
     "but there are many more: {}".format(", ".join(keys_added))
    )
```

Anyway, feel free to ignore this - I know that the 'ops' module is private and the 'select' argument is actually nowhere used by now (or it is used with the default `None` value). I was just reading through the code to see whether I will be able to create my own index (which I might need for a special reason). When I was reading through, I spotted this bug, and my colleague @matejak convinced me to create a pull request.
> Anyway, feel free to ignore this

For better or worse, we won't :smile:  An internal bug can easily surface at some point, so let's patch this if we can.  If you could, add the test as part of your PR, and then we can better review this.
Hello, it is great to hear that you appreciate our accomplishment and that you are committed to resolve this subtle issue at some point. We have briefly examined the possibility of writing tests that would expose this issue, but we have hit two problems:
* The function isn't documented, so even when it became obvious that something inside it was not right, we weren't 100% sure how a proper test of that function should look like, and
* we got totally lost concerning where to put those tests even if we had any. As far as I found out, no function from `ops.py` file has a unit test.

Remark - there is an alternative solution: The approach taken in this PR is based on guessing original author's (greetings to @jtratner ) intent and correcting the part that was arguably incorrect. An alternative solution would be getting rid of the select/exclude functionality of `add_methods` as the select part was  ineffective the whole time and there is a strong indication that even the exclude functionality is not used at all. This would lift the burden of having to write tests - no code = no tests :-)
@matejak : I think for now we should stick with just patching the functionality.  I think @HonzaOsobne wrote a pretty good test.  Here's what you can do: clean up the test and commit it as a separate file for now.  Then we can figure out where to put it.
can you demonstrate a user level bug where this shows?

otherwise I wouldn't even add a test, just do the change.
> otherwise I wouldn't even add a test, just do the change.

@jreback : Works for me.  I'll merge this in then given what @HonzaOsobne said earlier.
Thanks @HonzaOsobne !