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

Conversation26 Commits7 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 }})

Splitting up #21645

Added tests for duplicated, parametrized two tests for drop_duplicates, fixed a warning from #21614.

Codecov Report

Merging #21899 into master will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@ ## master #21899 +/- ##

Coverage 91.96% 91.96%

Files 166 166
Lines 50323 50323

Hits 46281 46281
Misses 4042 4042

Flag	Coverage Δ
#multiple	90.36% <ø> (ø)	⬆️
#single	42.22% <ø> (-0.01%)	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b51c96...e640040. Read the comment docs.

('last', Series([False, True, True, False, False, False, False])),
(False, Series([False, True, True, False, True, True, False]))
])
@pytest.mark.parametrize('npdtype', ['int_', 'uint', 'float_', 'unicode_'])

Choose a reason for hiding this comment

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

can you use the fixtures defined in pandas/conftest for the dtypes cc @gfyoung

Choose a reason for hiding this comment

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

@jreback
I tried

@pytest.mark.parametrize('npdtype', [any_int_dtype, float_dtype, string_dtype])

but pytest can't feed fixtures into parametrisation, see pytest-dev/pytest#349.

Not sure how to do this - separate tests?

Choose a reason for hiding this comment

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

you simply create a composite fixture

jreback added the Testing

pandas testing functions or related to the test suite

label

Jul 14, 2018

ALL_REAL_DTYPES = FLOAT_DTYPES + ALL_INT_DTYPES
ALL_NUMPY_DTYPES = ALL_REAL_DTYPES + STRING_DTYPES + COMPLEX_DTYPES

Choose a reason for hiding this comment

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

Make sure that these lists aren't duplicated in the file.

Choose a reason for hiding this comment

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

@gfyoung, what do you mean? I removed ALL_REAL_DTYPES from above and brought it closer to where it's used. Or do you just want one block of all-caps definitions?

Choose a reason for hiding this comment

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

One block of all-caps definitions and use it everywhere in the file. For example, search for :

float, "float32", "float64"

in your version of conftest.py . You'll see it exists twice in the file.

Choose a reason for hiding this comment

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

pls put these with the other definitions

('first', Series([False, False, False, False, True, True, False])),
('last', Series([False, True, True, False, False, False, False])),
(False, Series([False, True, True, False, True, True, False]))
])

Choose a reason for hiding this comment

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

either use 2 classes for test definitions or don’t use them (preferred)

Choose a reason for hiding this comment

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

same for frame

Choose a reason for hiding this comment

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

Classes of tests? I just wrote them in the same manner as in test_analytics and then transferred them here. You want me to get rid of TestSeriesDuplicates and do class-less tests - do I get that correctly?

Choose a reason for hiding this comment

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

yes that is the old format is much less readable

we only use classes if they r logical separations

Choose a reason for hiding this comment

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

for new modules we like to clean things up to conform

@@ -0,0 +1,145 @@
# coding=utf-8
# pylint: disable-msg=E1101,W0612

Choose a reason for hiding this comment

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

what are you disabling?

import pytest

import numpy as np
import pandas as pd

Choose a reason for hiding this comment

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

don’t import pd, directly import instead

from pandas import Series

from pandas.util.testing import assert_series_equal
import pandas.util.testing as tm

Choose a reason for hiding this comment

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

use tm; don’t import assert_series_equal

@jreback All just left-overs from the previous module. Hopefully converging on a clean module soon... ;-)

@h-vetinari absolutely - is just easier to fix when exracting things and make nice and clean

Choose a reason for hiding this comment

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

can u do the same for frame? (unique and duplicates)

@@ -248,7 +248,19 @@ def tz_aware_fixture(request):
return request.param


@pytest.fixture(params=[str, 'str', 'U'])
UNSIGNED_INT_DTYPES = ["uint8", "uint16", "uint32", "uint64"]
SIGNED_INT_DTYPES = [int, "int8", "int16", "int32", "int64"]

Choose a reason for hiding this comment

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

@gfyoung this is ok? it follows our existing pattern?

Choose a reason for hiding this comment

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

@jreback @gfyoung I only added COMPLEX_DTYPES, STRING_DTYPES (which was just giving names to existing collections) and ALL_NUMPY_DTYPES - the rest was just reordering.


import pytest

import numpy as np

Choose a reason for hiding this comment

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

you can remove space between pandas imports

can u do the same for frame? (unique and duplicates)

@jreback I did the same for frame in #21898

@jreback Actually, did the same for #21900 as well - e.g. replace pd. with direct imports

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

Jul 20, 2018

This was referenced

Sep 16, 2018

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

Oct 1, 2018

This was referenced

Nov 28, 2018

Labels

Testing

pandas testing functions or related to the test suite

TST/CLN: series.duplicated; parametrisation; fix warning by h-vetinari · Pull Request #21899 · pandas-dev/pandas (original) (raw)

Conversation

Codecov Report

@@ Coverage Diff @@ ## master #21899 +/- ##

Coverage 91.96% 91.96%

Files 166 166 Lines 50323 50323

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Files 166 166
Lines 50323 50323