pandas (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation35 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Split off from #22730 as per review from @WillAyd

The changes in conftest.py are due to the following:

In translating the quasi-fixtures from TestData to conftest in #22236, I sorted the dtypes for the columns of mixed_float_frame and mixed_int_frame, which turns out to have been a mistake. This is reverted here to be a true translation of the attribute of TestData. Otherwise, tests in the newly fixturized test_arithmetic.py would fail.

def test_arith_flex_frame(self):
seriesd = tm.getSeriesData()
frame = pd.DataFrame(seriesd).copy()
@pytest.mark.parametrize('op', ['add', 'sub', 'mul', 'div', 'truediv',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you change these to use the fixture: all_arithmetic_operators

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

intframe = pd.DataFrame({c: s for c, s in intframe.items()},
dtype=np.int64)

ops = ['add', 'sub', 'mul', 'div', 'truediv', 'pow', 'floordiv', 'mod']
if not PY3:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you prob don't need this (when using the fixture)

tm.assert_frame_equal(result, exp)
_check_mixed_float(result, dtype=dict(C=None))

# vs mix int

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be more clear to move these to a separate test (from here down)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to get into orthogonal changes here. I haven't changed anything about this except removing the indentation. This goes for the test split requested below as well.


const_add = frame.add(1)
tm.assert_frame_equal(const_add, frame + 1)
# ndim >= 3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and down could be in the top-section, e.g. the first test as they test all ops (or another tests is even better, with the fixture this becomes easy)

const_add = frame.add(1)
tm.assert_frame_equal(const_add, frame + 1)
# ndim >= 3
ndim_5 = np.ones(float_frame.shape + (3, 4, 5))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment here on what this is testing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above - I did not write the test; just removed indentation

@jreback

I though your request for all_arithmetic_operators would not change anything, but it includes more operators than previously tested - I get errors for all the reverse operators:

AttributeError: module 'operator' has no attribute '__rmod__'  [etc.]

I've skipped those tests now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ jreback

might be more clear to move these to a separate test (from here down)

can you add a comment here on what this is testing

OK, I relented, and split up and cleaned up that huge test. Also the skips for the r-versions of the ops aren't necessary anymore. They were tested for add/sub/mult before, and I extended that to all ops now.

tm.assert_frame_equal(result, exp)
_check_mixed_int(result, dtype=dtype)

# rops

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rops were only tested within that if-block. Now they're all tested

exp = f(intframe, 2 * intframe)
tm.assert_frame_equal(result, exp)

# vs mix int

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, this part was tested twice. only once in cleaned-up version

tm.assert_frame_equal(result, exp)
_check_mixed_float(result, dtype=dict(C=None))

@pytest.mark.parametrize('op', ['__add__', '__sub__', '__mul__'])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broke up the test like you wanted


op = all_arithmetic_operators

# Check that arrays with dim >= 3 raise

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment here on what this is testing

Added (and expanded)

msg = "Unable to coerce to Series/DataFrame"
with tm.assert_raises_regex(ValueError, msg):
f(frame, ndim_5)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part (testing via the f of the op) is removed because it's testing the same thing as testing the op from the frame directly below

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you separate out these dimension-specific tests?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split them out in a separate test

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok test looks a lot nicer. small change, ping on green.


# ndim >= 3
ndim_5 = np.ones(frame.shape + (3, 4, 5))
op = all_arithmetic_operators # one instance of parametrized fixture

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put comments on the prior line


if op.startswith('__r'):
# get op without "r" and invert it
tmp = getattr(operator, op[:2] + op[3:])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this a named function to avoid the tmp something like

def f(x, y):
    # comment
    op = op.replace('__r', '__')
    return getattr(operator, op)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment applies to the prior test actually. I don't think you need this here at all, as op is just add, mul, sub.

did you mean to test the reverse of these as well?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this a named function to avoid the tmp something like [...]

Best I can come up with is the following. Is this what you meant?

[...]
# one instance of parametrized fixture
op = all_arithmetic_operators

def f(x, y):
    if op.startswith('__r'):
        # get op without "r" and invert it
        return getattr(operator, op.replace('__r', '__'))(y, x)
    return getattr(operator, op)(x, y)
[...]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment applies to the prior test actually. I don't think you need this here at all, as op is just add, mul, sub. did you mean to test the reverse of these as well?

That was indeed not necessary anymore. Removed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok this is fine (your change). note that I suspect we do this in other places, maybe see if you can make a more generalized version of this that we can then include in the pandas.util.testing (but followon-PR)


if op.startswith('__r'):
# get op without "r" and invert it
tmp = getattr(operator, op[:2] + op[3:])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment applies to the prior test actually. I don't think you need this here at all, as op is just add, mul, sub.

did you mean to test the reverse of these as well?

def f(x, y):
if op.startswith('__r'):
# get op without "r" and invert it
return getattr(operator, op.replace('__r', '__'))(y, x)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. The alternative would be to do a lookup in core.ops, but I think for a reader who is unfamiliar with core.ops this is clearer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified comment slightly

# tm.assert_frame_equal(res_add, frame + frame)
# tm.assert_frame_equal(res_sub, frame - frame)
# tm.assert_frame_equal(res_mul, frame * frame)
# tm.assert_frame_equal(res_div, frame / (2 * frame))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this commented-out stuff go somewhere else? Or just determined to be redundant?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Determined to be redundant with test_arith_flex_frame, which checks all arith_ops, and much more thoroughly.

thanks @h-vetinari

so this is a good example of a change. you moved / parameterized things, but its a single change that is quite straightforward for a reviewer to grok / comment on w/o diving into things too much.

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request

Oct 1, 2018

Labels

Clean Testing

pandas testing functions or related to the test suite