Skip to content
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

Centralize m8[ns] Arithmetic Tests #22118

Merged
merged 4 commits into from
Jul 31, 2018
Merged

Conversation

jbrockmendel
Copy link
Member

There are a bunch still left in the TDI tests, but the diff is already pretty big as it is.

A ton of DataFrame tests are xfailed here. Most of these will be fixed by the follow-up to (#22068 + #22074).

xfails are made strict so that we get alerted when they are fixed. Several existing xfails are removed since they have been XPASSing for a while.

@jreback I know you're going to want to make fixtures for a bunch of things. I agree, but think that will be 19.6x easier after fixing some of the xfails. Also this is step 2 of a many-step process and in general I'd prefer to do cleanup after getting more stuff in place.

@codecov
Copy link

codecov bot commented Jul 30, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22118   +/-   ##
=======================================
  Coverage   92.07%   92.07%           
=======================================
  Files         170      170           
  Lines       50690    50690           
=======================================
  Hits        46672    46672           
  Misses       4018     4018
Flag Coverage Δ
#multiple 90.48% <ø> (ø) ⬆️
#single 42.31% <ø> (ø) ⬆️

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 d30c4a0...ede59e4. Read the comment docs.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

this looks ok, and yes was going to see if we can use fixtures, but can do that later. however, I think test_arithmetic is going to get big fast, maybe make a directory structure on this, e.g.

pandas/tests/arithmetic/test_timedelta...... etc.?

@jreback jreback added Testing pandas testing functions or related to the test suite Datetime Datetime data dtype Timedelta Timedelta data type labels Jul 30, 2018
@jbrockmendel
Copy link
Member Author

maybe make a directory structure on this

I've been thinking about this too. So far this file is all timedelta64[ns], so that's easy at least. I'll split it in an upcoming pass.

@jreback
Copy link
Contributor

jreback commented Jul 31, 2018

I've been thinking about this too. So far this file is all timedelta64[ns], so that's easy at least. I'll split it in an upcoming pass.

great, yah this will quickly explode otherwise i think.

@@ -2,14 +2,30 @@
# Arithmetc tests for DataFrame/Series/Index/Array classes that should
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add comments in the individual test_arithmetic that point back to this series (so future tests are put appropriately), can be in the future

# ------------------------------------------------------------------
# Fixtures

@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally centralize / cleanup fixtures, future pass ok

@@ -53,7 +69,7 @@ def test_index_mul_timedelta(self, scalar_td, index, box):
commute = scalar_td * index
tm.assert_equal(commute, expected)

@pytest.mark.parametrize('box', [pd.Index, pd.Series, pd.DataFrame])
@pytest.mark.parametrize('box', [pd.Index, Series, pd.DataFrame])
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this needs a fixture (future ok)

@jreback jreback added this to the 0.24.0 milestone Jul 31, 2018
@jreback jreback merged commit 7c67d9c into pandas-dev:master Jul 31, 2018
@jreback
Copy link
Contributor

jreback commented Jul 31, 2018

thanks. a few comments on future things to do.

@jbrockmendel jbrockmendel deleted the arith_tests3 branch July 31, 2018 15:59
dberenbaum pushed a commit to dberenbaum/pandas that referenced this pull request Aug 3, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Testing pandas testing functions or related to the test suite Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants