-
-
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
Fixturize tests/frame/test_dtypes.py #25636
Conversation
@@ -110,6 +110,18 @@ def mixed_int_frame(): | |||
return df | |||
|
|||
|
|||
@pytest.fixture | |||
def mixed_type_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.
so this fixtures itself is prob ok here. as we know that this is used in many places.
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.
on 2nd thought, let's see how often this is actually used, so move into the test module
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.
I'll reiterate from another thread, that with these fixturization PRs, I will:
- keep the
TestData
"fixtures" intests.frame.conftest
- any new fixtures will stay in their respective modules
- the effort for TST/CLN: remove TestData from frame-tests; replace with fixtures #22471 is strictly translation, no breakup of tests
- [reorg of fixtures from
TestData
into submodules to happen later]
pandas/tests/frame/test_dtypes.py
Outdated
expected = DataFrame(self.frame.values.astype(int), | ||
index=self.frame.index, | ||
columns=self.frame.columns) | ||
def test_astype(self, float_frame, mixed_float_frame, mixed_type_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.
see my comments elsewhere. don't use this pattern.
pandas/tests/frame/test_dtypes.py
Outdated
df['string'] = 'foo' | ||
casted = df.astype(np.int32, errors='ignore') | ||
|
||
expected['string'] = 'foo' | ||
assert_frame_equal(casted, expected) | ||
|
||
def test_astype_with_view(self): | ||
def test_astype_with_view(self, float_frame, mixed_float_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.
same
Codecov Report
@@ Coverage Diff @@
## master #25636 +/- ##
=======================================
Coverage 91.26% 91.26%
=======================================
Files 173 173
Lines 52968 52968
=======================================
Hits 48339 48339
Misses 4629 4629
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25636 +/- ##
==========================================
+ Coverage 41.95% 91.26% +49.3%
==========================================
Files 180 173 -7
Lines 50765 52982 +2217
==========================================
+ Hits 21300 48355 +27055
+ Misses 29465 4627 -24838
Continue to review full report at Codecov.
|
@jreback |
columns=['A', 'B']).astype('float16') | ||
_check_cast(casted, 'float16') | ||
|
||
def test_astype_mixed_type(self, mixed_type_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.
i don't want the repeated code.
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.
Fine by me, but that means this should be done in a separate PR. I'll move the _check_cast
function to the top of the module.
@@ -110,6 +110,18 @@ def mixed_int_frame(): | |||
return df | |||
|
|||
|
|||
@pytest.fixture | |||
def mixed_type_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.
on 2nd thought, let's see how often this is actually used, so move into the test module
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 points as in the other PRs...
@@ -110,6 +110,18 @@ def mixed_int_frame(): | |||
return df | |||
|
|||
|
|||
@pytest.fixture | |||
def mixed_type_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.
I'll reiterate from another thread, that with these fixturization PRs, I will:
- keep the
TestData
"fixtures" intests.frame.conftest
- any new fixtures will stay in their respective modules
- the effort for TST/CLN: remove TestData from frame-tests; replace with fixtures #22471 is strictly translation, no breakup of tests
- [reorg of fixtures from
TestData
into submodules to happen later]
columns=['A', 'B']).astype('float16') | ||
_check_cast(casted, 'float16') | ||
|
||
def test_astype_mixed_type(self, mixed_type_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.
Fine by me, but that means this should be done in a separate PR. I'll move the _check_cast
function to the top of the module.
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.
merge master
closing as stale |
I had merged master like you asked, this PR is not stale. Please reopen. |
@jreback @gfyoung @simonjayhawkins |
Thanks for reopening @simonjayhawkins. |
lgtm. |
thanks @h-vetinari |
One more steps towards #22471. Again, needing to re-add some fixtures that were removed in #24885.