-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TST/CLN: series.duplicated; parametrisation; fix warning #21899
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21899 +/- ##
=======================================
Coverage 91.96% 91.96%
=======================================
Files 166 166
Lines 50323 50323
=======================================
Hits 46281 46281
Misses 4042 4042
Continue to review full report at Codecov.
|
('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_']) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
needs a rebase |
d851ec9
to
2a649b6
Compare
2a649b6
to
c06442b
Compare
pandas/conftest.py
Outdated
ALL_REAL_DTYPES = FLOAT_DTYPES + ALL_INT_DTYPES | ||
ALL_NUMPY_DTYPES = ALL_REAL_DTYPES + STRING_DTYPES + COMPLEX_DTYPES | ||
|
||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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])) | ||
]) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for frame
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
f61e6a6
to
12d2888
Compare
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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"] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
import pytest | ||
|
||
import numpy as np |
There was a problem hiding this comment.
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
thanks @h-vetinari |
Splitting up #21645
Added tests for
duplicated
, parametrized two tests fordrop_duplicates
, fixed a warning from #21614.