TST: split up test_concat.py #37243 #37259
Conversation
There was a problem hiding this comment.
so i have request some reorg. the key here is to not touch inside tests at all, so that you can asssert number of tests before == number of tests after. I am happy for followups to touch up things is totally fine as well. but i think splitting out by dtype is useful here (and before merge).
| return request.param | ||
|
|
||
|
|
||
| class TestConcatenate: |
There was a problem hiding this comment.
i think this was a mistake originally, can you fold these into concat/test_concat.py
| result = concat([df.iloc[[0]], df.iloc[[1]]]) | ||
| tm.assert_series_equal(result.dtypes, df.dtypes) | ||
|
|
||
| def test_concat_series(self): |
There was a problem hiding this comment.
can you move these to test_series.py
|
|
||
| tm.assert_frame_equal(pd.concat(CustomIterator2(), ignore_index=True), expected) | ||
|
|
||
| def test_concat_invalid(self): |
There was a problem hiding this comment.
can you create a test_invalid.py (and move things that raise there)
There was a problem hiding this comment.
it might be more clear to do this as another pass (after merging this)
| y = [pd.Period("2011-03", freq="M"), pd.Period("2011-04", freq="M")] | ||
| result = concat([pd.Series(x), pd.Series(y)], ignore_index=True) | ||
| tm.assert_series_equal(result, pd.Series(x + y, dtype="object")) | ||
|
|
There was a problem hiding this comment.
would not be averse to creating a test_datetimes.py all datetime / tz related, can also split out a test_period and test_timedelta (if we have any of those)
| # it works | ||
| concat([df1, df2], sort=sort) | ||
|
|
||
| def test_handle_empty_objects(self, sort): |
There was a problem hiding this comment.
might be worthile to create a test_empty.py
There was a problem hiding this comment.
should I put also methods, which check empty, from test_series in test_empty? There are methods in test_series like:
def test_concat_empty_series_dtypes(self, left, right, expected):
result = pd.concat([Series(dtype=left), Series(dtype=right)])
def test_concat_empty_series_dtypes_roundtrips(self):
# round-tripping with self & like self
There was a problem hiding this comment.
yes ideally group all of teh empty tests together (whether dataframe / series).
| dfa = df1.append(df2) | ||
| tm.assert_index_equal(df["grade"].cat.categories, dfa["grade"].cat.categories) | ||
|
|
||
| def test_categorical_concat_preserve(self): |
| @@ -0,0 +1,274 @@ | |||
| from datetime import datetime | |||
There was a problem hiding this comment.
i would actually like to integrate these with the other tests (e.g. if they are tz related then into test_datetimes.py), but of some won't and you can leave if its more clear here or put in test_concat either way)
jreback
added
Reshaping
…fite56/pandas into issue37243_split_test_concat
…sue37243_split_test_concat
|
Hello @laffite56! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-10-20 17:03:34 UTC |
|
I have put a few functions into classes, but i don't know if this is fine. |
I generally would go the other way actually. But most important for this PR. please let's do just straight copy-paste, its very hard to see changes otherwise. my requests were all about just moving existing code around in blocks, not actually change that much. so we want to merge this soon. I am also happy to do this in a separate PR by first literally moving the entire file, then moving pieces of it. |
|
I'm a little confused. You want me to create a new PR, with one commit containing only copy-paste original "test_concat.py" to new folder, like i have right now: from pandas/tests/reshape/test_concat.py => pandas/tests/reshape/concat/test_concat.py and then in next commit reshape "test_concat.py" to submodules? |
|
Sir, @jreback, i don't really know what to do with this issue. I can easly make new PR with every commit separated, like one commit for moving one file, then next ones for every new submodule and for every significant change, so You'll be able to see every change. |
so 1) pls merge master. I would actually just do a straight move & split and NOTHING else in a the first pr. split by classes is fine. |
|
#37360 here is new PR. |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff